diff --git a/ZB.MOM.WW.Auth/Directory.Build.props b/ZB.MOM.WW.Auth/Directory.Build.props index f03af92..46a66f9 100644 --- a/ZB.MOM.WW.Auth/Directory.Build.props +++ b/ZB.MOM.WW.Auth/Directory.Build.props @@ -5,7 +5,10 @@ enable enable latest - 0.2.0 + + 0.2.1 true diff --git a/ZB.MOM.WW.Auth/src/ZB.MOM.WW.Auth.Ldap/Internal/NovellLdapConnection.cs b/ZB.MOM.WW.Auth/src/ZB.MOM.WW.Auth.Ldap/Internal/NovellLdapConnection.cs index 3c684a7..839b188 100644 --- a/ZB.MOM.WW.Auth/src/ZB.MOM.WW.Auth.Ldap/Internal/NovellLdapConnection.cs +++ b/ZB.MOM.WW.Auth/src/ZB.MOM.WW.Auth.Ldap/Internal/NovellLdapConnection.cs @@ -99,23 +99,73 @@ internal sealed class NovellLdapConnection : ILdapConnection attributes.ToArray(), typesOnly: false); + return ReadEntries(results); + } + + /// + /// Drains a result set into entries, skipping continuation referrals. + /// + /// + /// + /// Active Directory answers any search based at a domain root with one or more + /// continuation references (to CN=Configuration, DC=ForestDnsZones and + /// friends). With referral following disabled — the default, and what we want, since + /// chasing a referral would re-bind somewhere we did not authenticate — Novell surfaces + /// each one by throwing from . + /// + /// + /// A continuation reference is a pointer to another naming context, not an error: + /// it says "there may be more results over there", never that this search failed. Letting + /// it escape made the whole search stage fail, which LdapAuthService maps to + /// — so against a + /// real AD domain root, every login failed with a system-side error while the directory, + /// the credentials and both DCs were perfectly healthy. Verified against + /// DC=nam,DC=zimmer,DC=com on 2026-08-12. + /// + /// + /// Skipping is the correct behaviour rather than a workaround: the entries we can see are + /// exactly the ones in the naming context we bound to, which is the set the caller asked + /// about. Internal so the referral handling can be unit-tested through the + /// seam without a live directory. + /// + /// + internal static List ReadEntries(ILdapSearchResults results) + { var entries = new List(); - while (results.HasMore()) + var referrals = 0; + + while (true) { - var entry = results.Next(); - var attrs = new Dictionary>(StringComparer.OrdinalIgnoreCase); - - foreach (LdapAttribute attr in entry.GetAttributeSet()) + try { - attrs[attr.Name] = attr.StringValueArray.ToList(); - } + if (!results.HasMore()) break; - entries.Add(new LdapSearchEntry(entry.Dn, attrs)); + var entry = results.Next(); + var attrs = new Dictionary>(StringComparer.OrdinalIgnoreCase); + + foreach (LdapAttribute attr in entry.GetAttributeSet()) + { + attrs[attr.Name] = attr.StringValueArray.ToList(); + } + + entries.Add(new LdapSearchEntry(entry.Dn, attrs)); + } + catch (LdapException ex) when (ex is LdapReferralException || ex.ResultCode == LdapException.Referral) + { + // Novell dequeues the reference before throwing, so the enumeration advances and + // this terminates. The cap is a belt-and-braces guard: a client that threw without + // advancing would otherwise spin forever inside a fail-closed auth path, which is a + // far worse failure than under-reporting an implausible number of referrals. + if (++referrals > MaxReferrals) break; + } } return entries; } + /// Upper bound on continuation references tolerated in one search — see . + private const int MaxReferrals = 1_000; + /// public void Dispose() { diff --git a/ZB.MOM.WW.Auth/tests/ZB.MOM.WW.Auth.Ldap.Tests/NovellSearchReferralTests.cs b/ZB.MOM.WW.Auth/tests/ZB.MOM.WW.Auth.Ldap.Tests/NovellSearchReferralTests.cs new file mode 100644 index 0000000..1d0f683 --- /dev/null +++ b/ZB.MOM.WW.Auth/tests/ZB.MOM.WW.Auth.Ldap.Tests/NovellSearchReferralTests.cs @@ -0,0 +1,179 @@ +using Novell.Directory.Ldap; +using Xunit; +using ZB.MOM.WW.Auth.Ldap.Internal; + +namespace ZB.MOM.WW.Auth.Ldap.Tests; + +/// +/// Covers ' handling of AD continuation +/// referrals. Active Directory returns one for any search based at a domain root, and +/// Novell surfaces it by throwing from Next(). Before this was handled, the +/// exception escaped the search stage and every login against +/// DC=nam,DC=zimmer,DC=com failed as ServiceAccountBindFailed while the +/// directory and credentials were healthy. +/// +public sealed class NovellSearchReferralTests +{ + [Fact] + public void Referral_before_the_only_entry_does_not_lose_it() + { + var results = new ScriptedResults( + Step.Referral(), + Step.Entry("CN=dohertj2,OU=Office Users,OU=Warsaw,DC=nam,DC=zimmer,DC=com")); + + var entries = NovellLdapConnection.ReadEntries(results); + + var entry = Assert.Single(entries); + Assert.Equal("CN=dohertj2,OU=Office Users,OU=Warsaw,DC=nam,DC=zimmer,DC=com", entry.Dn); + } + + [Fact] + public void Referral_after_the_entry_does_not_discard_what_was_already_read() + { + var results = new ScriptedResults( + Step.Entry("CN=a,DC=nam,DC=zimmer,DC=com"), + Step.Referral()); + + var entries = NovellLdapConnection.ReadEntries(results); + + Assert.Equal("CN=a,DC=nam,DC=zimmer,DC=com", Assert.Single(entries).Dn); + } + + /// + /// The real shape observed against the NAM domain root: several references interleaved + /// with the one user entry. All three of AD's usual continuation points are present. + /// + [Fact] + public void Multiple_interleaved_referrals_still_yield_every_entry() + { + var results = new ScriptedResults( + Step.Referral(), + Step.Entry("CN=a,DC=nam,DC=zimmer,DC=com"), + Step.Referral(), + Step.Referral(), + Step.Entry("CN=b,DC=nam,DC=zimmer,DC=com")); + + var entries = NovellLdapConnection.ReadEntries(results); + + Assert.Equal(["CN=a,DC=nam,DC=zimmer,DC=com", "CN=b,DC=nam,DC=zimmer,DC=com"], + entries.Select(e => e.Dn)); + } + + /// + /// A referral-only result set is a legitimate "nothing here" answer, and must come back + /// empty rather than throwing — LdapAuthService reads empty as UserNotFound, + /// which is the honest verdict, where an exception would become a system-side failure. + /// + [Fact] + public void Referrals_only_returns_empty_rather_than_throwing() + { + var results = new ScriptedResults(Step.Referral(), Step.Referral()); + + Assert.Empty(NovellLdapConnection.ReadEntries(results)); + } + + /// + /// A base carrying the Referral result code is treated the + /// same as the derived type — the catch filter must not depend on the exact class. + /// + [Fact] + public void Referral_result_code_on_a_base_exception_is_also_skipped() + { + var results = new ScriptedResults( + Step.Throwing(new LdapException("referral", LdapException.Referral, null)), + Step.Entry("CN=a,DC=nam,DC=zimmer,DC=com")); + + Assert.Single(NovellLdapConnection.ReadEntries(results)); + } + + /// + /// Only referrals are swallowed. A genuine directory fault must still propagate so the + /// caller maps it to a system-side failure — silently returning a short result set would + /// turn "the directory is broken" into "no such user", which is the wrong answer to give + /// about someone's credentials. + /// + [Fact] + public void A_non_referral_fault_still_propagates() + { + var results = new ScriptedResults( + Step.Throwing(new LdapException("busy", LdapException.Busy, null)), + Step.Entry("CN=a,DC=nam,DC=zimmer,DC=com")); + + var ex = Assert.Throws(() => NovellLdapConnection.ReadEntries(results)); + Assert.Equal(LdapException.Busy, ex.ResultCode); + } + + /// + /// A pathological client that throws a referral without advancing must not spin forever + /// inside a fail-closed authentication path. + /// + [Fact] + public void An_endless_referral_stream_terminates() + { + var results = new EndlessReferrals(); + + var entries = NovellLdapConnection.ReadEntries(results); + + Assert.Empty(entries); + Assert.InRange(results.Calls, 1, 2_000); + } + + // --------------------------------------------------------------------- + + private sealed record Step(LdapEntry? Value, Exception? Throw) + { + public static Step Entry(string dn) => new(new LdapEntry(dn, new LdapAttributeSet()), null); + public static Step Referral() => new(null, new LdapReferralException("continuation reference")); + public static Step Throwing(Exception ex) => new(null, ex); + } + + /// Replays a fixed script of entries and faults through the Novell result seam. + private sealed class ScriptedResults : ILdapSearchResults + { + private readonly Step[] _steps; + private int _index; + + public ScriptedResults(params Step[] steps) => _steps = steps; + + public int Count => _steps.Length; + + public LdapControl[] ResponseControls => []; + + public bool HasMore() => _index < _steps.Length; + + public LdapEntry Next() + { + var step = _steps[_index++]; + if (step.Throw is not null) throw step.Throw; + return step.Value!; + } + + // The production reader drives HasMore()/Next() directly; enumeration is part of the + // interface but not part of the path under test. + public IEnumerator GetEnumerator() => throw new NotSupportedException(); + + System.Collections.IEnumerator System.Collections.IEnumerable.GetEnumerator() => GetEnumerator(); + } + + /// Always claims more results and always throws a referral, never advancing. + private sealed class EndlessReferrals : ILdapSearchResults + { + public int Calls { get; private set; } + + public int Count => int.MaxValue; + + public LdapControl[] ResponseControls => []; + + public bool HasMore() => true; + + public LdapEntry Next() + { + Calls++; + throw new LdapReferralException("continuation reference"); + } + + public IEnumerator GetEnumerator() => throw new NotSupportedException(); + + System.Collections.IEnumerator System.Collections.IEnumerable.GetEnumerator() => GetEnumerator(); + } +}