From 7db7e42182dcb74b2ef410af91da37ba297f5326 Mon Sep 17 00:00:00 2001 From: rvazarkar Date: Thu, 23 Apr 2026 18:44:26 -0400 Subject: [PATCH 01/20] feat: comprehensive replacement for the GetDomain calls to close one of the biggest ADSI gaps, as well as several improvements to the logic --- src/CommonLib/ConnectionPoolManager.cs | 26 +- src/CommonLib/DomainInfo.cs | 35 + src/CommonLib/Enums/LDAPProperties.cs | 2 + src/CommonLib/ILdapUtils.cs | 19 + src/CommonLib/LdapConfig.cs | 6 + src/CommonLib/LdapConnectionPool.cs | 68 +- src/CommonLib/LdapUtils.cs | 1009 ++++++++++++++++- .../Processors/GPOLocalGroupProcessor.cs | 7 +- test/unit/Facades/MockLdapUtils.cs | 8 + test/unit/GPOLocalGroupProcessorTest.cs | 4 +- test/unit/LDAPUtilsTest.cs | 148 +++ 11 files changed, 1243 insertions(+), 89 deletions(-) create mode 100644 src/CommonLib/DomainInfo.cs diff --git a/src/CommonLib/ConnectionPoolManager.cs b/src/CommonLib/ConnectionPoolManager.cs index 61136cc7f..a01bcf8dd 100644 --- a/src/CommonLib/ConnectionPoolManager.cs +++ b/src/CommonLib/ConnectionPoolManager.cs @@ -1,8 +1,6 @@ using System; using System.Collections.Concurrent; using System.Collections.Generic; -using System.DirectoryServices; -using System.Runtime.CompilerServices; using System.Security.Principal; using System.Threading; using System.Threading.Tasks; @@ -146,17 +144,19 @@ private string ResolveIdentifier(string identifier) { //we expect this to fail sometimes } - if (LdapUtils.GetDomain(domainName, _ldapConfig, out var domainObject)) - try { - // TODO: MC - Confirm GetDirectoryEntry is not a Blocking External Call - if (domainObject.GetDirectoryEntry().ToDirectoryObject().TryGetSecurityIdentifier(out domainSid)) { - Cache.AddDomainSidMapping(domainName, domainSid); - return (true, domainSid); - } - } - catch { - //we expect this to fail sometimes (not sure why, but better safe than sorry) - } + // Controlled replacement for LdapUtils.GetDomain + GetDirectoryEntry. We pass pool: null + // because this method is called from inside GetPool -> ResolveIdentifier while resolving + // the pool for this same domain; reusing the pool here would reenter GetLdapConnection and + // recurse into GetDomainSidFromDomainName. With pool: null, GetDomainInfoStaticAsync falls + // through to its direct-LDAP (one-shot LdapConnection) path, which still honors LdapConfig. + // The call is sync-over-async to match the sibling pattern in GetLdapConnectionForServer. + var (infoOk, info) = LdapUtils + .GetDomainInfoStaticAsync(domainName, _ldapConfig, _log) + .GetAwaiter().GetResult(); + if (infoOk && !string.IsNullOrEmpty(info?.DomainSid)) { + Cache.AddDomainSidMapping(domainName, info.DomainSid); + return (true, info.DomainSid); + } foreach (var name in _translateNames) try { diff --git a/src/CommonLib/DomainInfo.cs b/src/CommonLib/DomainInfo.cs new file mode 100644 index 000000000..adaa1196c --- /dev/null +++ b/src/CommonLib/DomainInfo.cs @@ -0,0 +1,35 @@ +using System; +using System.Collections.Generic; + +namespace SharpHoundCommonLib +{ + /// + /// Lightweight, transport-agnostic description of an Active Directory domain populated either + /// from controlled LDAP queries (honoring ) or, when explicitly opted in + /// via , from + /// System.DirectoryServices.ActiveDirectory.Domain.GetDomain. + /// + public sealed class DomainInfo + { + /// Upper-cased DNS name of the domain (e.g. CONTOSO.LOCAL). + public string Name { get; set; } + + /// Default naming context distinguished name (e.g. DC=contoso,DC=local). + public string DistinguishedName { get; set; } + + /// Upper-cased DNS name of the forest root domain, when known. + public string ForestName { get; set; } + + /// Domain SID (S-1-5-21-...) if resolved, otherwise null. + public string DomainSid { get; set; } + + /// Legacy NetBIOS domain name if resolved from the Partitions container, otherwise null. + public string NetBiosName { get; set; } + + /// DNS hostname of the PDC FSMO role owner if resolved, otherwise null. + public string PrimaryDomainController { get; set; } + + /// DNS hostnames of known domain controllers for this domain. + public IReadOnlyList DomainControllers { get; set; } = Array.Empty(); + } +} diff --git a/src/CommonLib/Enums/LDAPProperties.cs b/src/CommonLib/Enums/LDAPProperties.cs index 0bf6b726e..b9ee6f53e 100644 --- a/src/CommonLib/Enums/LDAPProperties.cs +++ b/src/CommonLib/Enums/LDAPProperties.cs @@ -96,5 +96,7 @@ public static class LDAPProperties public const string LockOutObservationWindow = "lockoutobservationwindow"; public const string PrincipalName = "msds-principalname"; public const string GroupType = "grouptype"; + public const string FSMORoleOwner = "fsmoroleowner"; + public const string NCName = "ncname"; } } diff --git a/src/CommonLib/ILdapUtils.cs b/src/CommonLib/ILdapUtils.cs index 753e53bd1..a6b17828d 100644 --- a/src/CommonLib/ILdapUtils.cs +++ b/src/CommonLib/ILdapUtils.cs @@ -89,6 +89,25 @@ IAsyncEnumerable> RangedRetrieval(string distinguishedName, /// True if the domain was found, false if not bool GetDomain(out System.DirectoryServices.ActiveDirectory.Domain domain); + /// + /// Resolves a for the specified domain using controlled LDAP queries + /// that honor the configured (server, port, SSL, auth, signing, cert verification). + /// Falls back to System.DirectoryServices.ActiveDirectory.Domain.GetDomain only when + /// is enabled. + /// + /// The domain name to resolve + /// A tuple containing success state as well as the populated DomainInfo if successful + Task<(bool Success, DomainInfo DomainInfo)> GetDomainInfoAsync(string domainName); + + /// + /// Resolves a for the user's current domain using controlled LDAP queries + /// that honor the configured . Falls back to + /// System.DirectoryServices.ActiveDirectory.Domain.GetDomain only when + /// is enabled. + /// + /// A tuple containing success state as well as the populated DomainInfo if successful + Task<(bool Success, DomainInfo DomainInfo)> GetDomainInfoAsync(); + Task<(bool Success, string ForestName)> GetForest(string domain); /// /// Attempts to resolve an account name to its corresponding typed principal diff --git a/src/CommonLib/LdapConfig.cs b/src/CommonLib/LdapConfig.cs index 19e9995d0..6c33adb0a 100644 --- a/src/CommonLib/LdapConfig.cs +++ b/src/CommonLib/LdapConfig.cs @@ -15,6 +15,8 @@ public class LdapConfig public bool DisableCertVerification { get; set; } = false; public AuthType AuthType { get; set; } = AuthType.Kerberos; public int MaxConcurrentQueries { get; set; } = 15; + public bool AllowFallbackToUncontrolledLdap { get; set; } = false; + public string CurrentUserDomain { get; set; } = null; //Returns the port for connecting to LDAP. Will always respect a user's overridden config over anything else public int GetPort(bool ssl) @@ -56,6 +58,10 @@ public override string ToString() { sb.AppendLine($"ForceSSL: {ForceSSL}"); sb.AppendLine($"AuthType: {AuthType.ToString()}"); sb.AppendLine($"MaxConcurrentQueries: {MaxConcurrentQueries}"); + sb.AppendLine($"AllowFallbackToUncontrolledLdap: {AllowFallbackToUncontrolledLdap}"); + if (!string.IsNullOrWhiteSpace(CurrentUserDomain)) { + sb.AppendLine($"CurrentUserDomain: {CurrentUserDomain}"); + } if (!string.IsNullOrWhiteSpace(Username)) { sb.AppendLine($"Username: {Username}"); } diff --git a/src/CommonLib/LdapConnectionPool.cs b/src/CommonLib/LdapConnectionPool.cs index 4744cb402..fff843d9c 100644 --- a/src/CommonLib/LdapConnectionPool.cs +++ b/src/CommonLib/LdapConnectionPool.cs @@ -1,7 +1,6 @@ using System; using System.Collections.Concurrent; using System.Collections.Generic; -using System.DirectoryServices.ActiveDirectory; using System.DirectoryServices.Protocols; using System.Linq; using System.Net; @@ -451,7 +450,7 @@ private async Task SetupLdapQuery(LdapQueryParameters quer return result; } - var (searchRequestSuccess, searchRequest) = CreateSearchRequest(queryParameters, connectionWrapper); + var (searchRequestSuccess, searchRequest) = await CreateSearchRequestAsync(queryParameters, connectionWrapper); if (!searchRequestSuccess) { result.Success = false; result.Message = "Failed to create search request"; @@ -492,7 +491,7 @@ public async IAsyncEnumerable> RangedRetrieval(string distinguish }; var connectionWrapper = connectionResult.ConnectionWrapper; - var (searchRequestSuccess, searchRequest) = CreateSearchRequest(queryParameters, connectionWrapper); + var (searchRequestSuccess, searchRequest) = await CreateSearchRequestAsync(queryParameters, connectionWrapper); if (!searchRequestSuccess) { ReleaseConnection(connectionWrapper); yield return Result.Fail("Failed to create search request"); @@ -685,23 +684,30 @@ private static TimeSpan GetNextBackoff(int retryCount) { MaxBackoffDelay.TotalSeconds)); } - private (bool, SearchRequest) CreateSearchRequest(LdapQueryParameters queryParameters, + private async Task<(bool, SearchRequest)> CreateSearchRequestAsync(LdapQueryParameters queryParameters, LdapConnectionWrapper connectionWrapper) { string basePath; if (!string.IsNullOrWhiteSpace(queryParameters.SearchBase)) { basePath = queryParameters.SearchBase; - } - else if (!connectionWrapper.GetSearchBase(queryParameters.NamingContext, out basePath)) { + } else if (!connectionWrapper.GetSearchBase(queryParameters.NamingContext, out basePath)) { string tempPath; if (CallDsGetDcName(queryParameters.DomainName, out var info) && info != null) { tempPath = Helpers.DomainNameToDistinguishedName(info.Value.DomainName); connectionWrapper.SaveContext(queryParameters.NamingContext, basePath); } - else if (LdapUtils.GetDomain(queryParameters.DomainName, _ldapConfig, out var domainObject)) { - tempPath = Helpers.DomainNameToDistinguishedName(domainObject.Name); - } else { - return (false, null); + // Controlled replacement for LdapUtils.GetDomain + DomainNameToDistinguishedName. + // Pass null for the pool: this code runs *inside* an LdapConnectionPool, so + // attempting controlled resolution via the pool would reenter us. The static + // helper will fall through to the uncontrolled fallback, which itself honors + // LdapConfig.AllowFallbackToUncontrolledLdap. + var (ok, domainInfo) = await LdapUtils + .GetDomainInfoStaticAsync(queryParameters.DomainName, _ldapConfig, _log); + if (!ok || string.IsNullOrWhiteSpace(domainInfo?.DistinguishedName)) { + return (false, null); + } + + tempPath = domainInfo.DistinguishedName; } basePath = queryParameters.NamingContext switch { @@ -873,16 +879,20 @@ await CreateLdapConnection(tempDomainName, globalCatalog) is (true, var connecti } } - if (!LdapUtils.GetDomain(_identifier, _ldapConfig, out var domainObject) || domainObject?.Name == null) { + // Controlled replacement for LdapUtils.GetDomain. The static helper still honors + // LdapConfig.AllowFallbackToUncontrolledLdap for the uncontrolled fallback. + var (infoOk, info) = + await LdapUtils.GetDomainInfoStaticAsync(_identifier, _ldapConfig, _log); + if (!infoOk || string.IsNullOrEmpty(info?.Name)) { //If we don't get a result here, we effectively have no other ways to resolve this domain, so we'll just have to exit out _log.LogDebug( - "Could not get domain object from GetDomain, unable to create ldap connection for domain {Domain}", + "Could not resolve domain info, unable to create ldap connection for domain {Domain}", _identifier); ExcludedDomains.Add(_identifier); - return (false, null, "Unable to get domain object for further strategies"); + return (false, null, "Unable to get domain info for further strategies"); } - tempDomainName = domainObject.Name.ToUpper().Trim(); + tempDomainName = info.Name.ToUpper().Trim(); if (!tempDomainName.Equals(_identifier, StringComparison.OrdinalIgnoreCase) && await CreateLdapConnection(tempDomainName, globalCatalog) is (true, var connectionWrapper4)) { @@ -892,24 +902,26 @@ await CreateLdapConnection(tempDomainName, globalCatalog) is (true, var connecti return (true, connectionWrapper4, ""); } - var primaryDomainController = domainObject.PdcRoleOwner.Name; - var portConnectionResult = - await CreateLDAPConnectionWithPortCheck(primaryDomainController, globalCatalog); - if (portConnectionResult.success) { - _log.LogDebug( - "Successfully created ldap connection for domain: {Domain} using strategy 5 with to pdc {Server}", - _identifier, primaryDomainController); - return (true, portConnectionResult.connection, ""); + if (!string.IsNullOrEmpty(info.PrimaryDomainController)) { + var primaryDomainController = info.PrimaryDomainController; + var portConnectionResult = + await CreateLDAPConnectionWithPortCheck(primaryDomainController, globalCatalog); + if (portConnectionResult.success) { + _log.LogDebug( + "Successfully created ldap connection for domain: {Domain} using strategy 5 with to pdc {Server}", + _identifier, primaryDomainController); + return (true, portConnectionResult.connection, ""); + } } - // Blocking External Call - Possible on domainObject.DomainControllers as it calls DsGetDcNameWrapper - foreach (DomainController dc in domainObject.DomainControllers) { - portConnectionResult = - await CreateLDAPConnectionWithPortCheck(dc.Name, globalCatalog); + foreach (var dcName in info.DomainControllers) { + if (string.IsNullOrEmpty(dcName)) continue; + var portConnectionResult = + await CreateLDAPConnectionWithPortCheck(dcName, globalCatalog); if (portConnectionResult.success) { _log.LogDebug( - "Successfully created ldap connection for domain: {Domain} using strategy 6 with to pdc {Server}", - _identifier, primaryDomainController); + "Successfully created ldap connection for domain: {Domain} using strategy 6 to dc {Server}", + _identifier, dcName); return (true, portConnectionResult.connection, ""); } } diff --git a/src/CommonLib/LdapUtils.cs b/src/CommonLib/LdapUtils.cs index 2ce52dd3e..d1272471d 100644 --- a/src/CommonLib/LdapUtils.cs +++ b/src/CommonLib/LdapUtils.cs @@ -4,6 +4,7 @@ using System.DirectoryServices; using System.DirectoryServices.AccountManagement; using System.DirectoryServices.ActiveDirectory; +using System.DirectoryServices.Protocols; using System.Linq; using System.Net; using System.Net.Sockets; @@ -31,12 +32,21 @@ namespace SharpHoundCommonLib { public class LdapUtils : ILdapUtils { //This cache is indexed by domain sid private static ConcurrentDictionary _domainCache = new(); + private static ConcurrentDictionary _domainInfoCache = + new(StringComparer.OrdinalIgnoreCase); private static ConcurrentHashSet _domainControllers = new(StringComparer.OrdinalIgnoreCase); private static ConcurrentHashSet _unresolvablePrincipals = new(StringComparer.OrdinalIgnoreCase); private static readonly ConcurrentDictionary DomainToForestCache = new(StringComparer.OrdinalIgnoreCase); + // Single-shot cache for the uncontrolled Domain.GetDomain() hint tier in + // ResolveEffectiveDomainHint. null = not attempted; "" = attempted and failed; otherwise the + // resolved domain name. Guarded by _uncontrolledGetDomainHintLock because the underlying RPC + // is slow and we only want it to fire once per process. Reset from ResetUtils. + private static string _uncontrolledGetDomainHint; + private static readonly object _uncontrolledGetDomainHintLock = new(); + private static readonly ConcurrentDictionary SeenWellKnownPrincipals = new(); @@ -52,7 +62,7 @@ private readonly ConcurrentDictionary // Metrics private readonly IMetricRouter _metric; - + private readonly ILogger _log; private readonly IPortScanner _portScanner; private readonly NativeMethods _nativeMethods; @@ -280,8 +290,9 @@ public IAsyncEnumerable> PagedQuery(LdapQueryParame if (!securityIdentifier.Equals("S-1-5-9", StringComparison.OrdinalIgnoreCase)) { var tempDomain = domain; - if (GetDomain(tempDomain, out var domainObject) && domainObject.Name != null) { - tempDomain = domainObject.Name; + if (await GetDomainInfoAsync(tempDomain) is (true, var domainInfo) && + !string.IsNullOrEmpty(domainInfo?.Name)) { + tempDomain = domainInfo.Name; } return ($"{tempDomain}-{securityIdentifier}".ToUpper(), tempDomain); @@ -301,15 +312,10 @@ public IAsyncEnumerable> PagedQuery(LdapQueryParame return (true, cachedForest); } - if (GetDomain(domain, out var domainObject)) { - try { - var forestName = domainObject.Forest.Name.ToUpper(); - DomainToForestCache.TryAdd(domain, forestName); - return (true, forestName); - } - catch { - //pass - } + if (await GetDomainInfoAsync(domain) is (true, var domainInfo) && + !string.IsNullOrEmpty(domainInfo?.ForestName)) { + DomainToForestCache.TryAdd(domain, domainInfo.ForestName); + return (true, domainInfo.ForestName); } var (success, forest) = await GetForestFromLdap(domain); @@ -395,12 +401,13 @@ public IAsyncEnumerable> PagedQuery(LdapQueryParame } private async Task<(bool Success, string DomainName)> ConvertDomainSidToDomainNameFromLdap(string domainSid) { - if (!GetDomain(out var domain) || domain?.Name == null) { + var (domainOk, domainInfo) = await GetDomainInfoAsync(); + if (!domainOk || string.IsNullOrEmpty(domainInfo?.Name)) { return (false, string.Empty); } var result = await Query(new LdapQueryParameters { - DomainName = domain.Name, + DomainName = domainInfo.Name, Attributes = new[] { LDAPProperties.DistinguishedName }, GlobalCatalog = true, LDAPFilter = new LdapFilter().AddDomains(CommonFilters.SpecificSID(domainSid)).GetFilter() @@ -411,7 +418,7 @@ public IAsyncEnumerable> PagedQuery(LdapQueryParame } result = await Query(new LdapQueryParameters { - DomainName = domain.Name, + DomainName = domainInfo.Name, Attributes = new[] { LDAPProperties.DistinguishedName, LDAPProperties.Name }, GlobalCatalog = true, LDAPFilter = new LdapFilter().AddFilter("(objectclass=trusteddomain)", true) @@ -423,7 +430,7 @@ public IAsyncEnumerable> PagedQuery(LdapQueryParame } result = await Query(new LdapQueryParameters { - DomainName = domain.Name, + DomainName = domainInfo.Name, Attributes = new[] { LDAPProperties.DistinguishedName }, LDAPFilter = new LdapFilter().AddFilter("(objectclass=domaindns)", true) .AddFilter(CommonFilters.SpecificSID(domainSid), true).GetFilter() @@ -452,17 +459,13 @@ public IAsyncEnumerable> PagedQuery(LdapQueryParame //we expect this to fail sometimes } - if (GetDomain(domainName, out var domainObject)) - try { - var entry = domainObject.GetDirectoryEntry().ToDirectoryObject(); - if (entry.TryGetSecurityIdentifier(out domainSid)) { - Cache.AddDomainSidMapping(domainName, domainSid); - return (true, domainSid); - } - } - catch { - //we expect this to fail sometimes (not sure why, but better safe than sorry) - } + // Replaces the legacy GetDomain(out Domain) + GetDirectoryEntry block with a + // controlled lookup via the connection pool. + if (await GetDomainInfoAsync(domainName) is (true, var domainInfo) && + !string.IsNullOrEmpty(domainInfo?.DomainSid)) { + Cache.AddDomainSidMapping(domainName, domainInfo.DomainSid); + return (true, domainInfo.DomainSid); + } foreach (var name in _translateNames) try { @@ -499,6 +502,14 @@ public IAsyncEnumerable> PagedQuery(LdapQueryParame /// /// public bool GetDomain(string domainName, out Domain domain) { + if (!_ldapConfig.AllowFallbackToUncontrolledLdap) { + _log.LogDebug( + "GetDomain(\"{Name}\", out Domain) short-circuited: AllowFallbackToUncontrolledLdap is disabled", + domainName); + domain = null; + return false; + } + var cacheKey = domainName ?? _nullCacheKey; if (_domainCache.TryGetValue(cacheKey, out domain)) return true; @@ -529,6 +540,14 @@ public bool GetDomain(string domainName, out Domain domain) { } public static bool GetDomain(string domainName, LdapConfig ldapConfig, out Domain domain) { + if (ldapConfig == null || !ldapConfig.AllowFallbackToUncontrolledLdap) { + Logging.Logger.LogDebug( + "Static GetDomain(\"{DomainName}\") short-circuited: AllowFallbackToUncontrolledLdap is disabled", + domainName); + domain = null; + return false; + } + if (_domainCache.TryGetValue(domainName, out domain)) return true; try { @@ -566,6 +585,13 @@ public static bool GetDomain(string domainName, LdapConfig ldapConfig, out Domai /// /// public bool GetDomain(out Domain domain) { + if (!_ldapConfig.AllowFallbackToUncontrolledLdap) { + _log.LogDebug( + "GetDomain(out Domain) short-circuited: AllowFallbackToUncontrolledLdap is disabled"); + domain = null; + return false; + } + if (_domainCache.TryGetValue(_nullCacheKey, out domain)) return true; try { @@ -724,7 +750,7 @@ public bool GetDomain(out Domain domain) { _log.LogTrace("CheckPort returned false for {HostName}.", hostname); return (false, default); } - + // Blocking External Call var result = await _callNetWkstaGetInfoAdaptiveTimeout.ExecuteNetAPIWithTimeout((_) => _nativeMethods.CallNetWkstaGetInfo(hostname)); @@ -1105,42 +1131,939 @@ public void SetLdapConfig(LdapConfig config) { //pass } - if (GetDomain(domain, out var domainObj)) { + // Controlled replacement for the old GetDomain(out Domain) + DomainNameToDistinguishedName + // block. DomainInfo.DistinguishedName is the default naming context DN; Configuration and + // Schema NCs are constructed from it the same way the old path constructed them. + if (await GetDomainInfoAsync(domain) is (true, var domainInfo) && + !string.IsNullOrWhiteSpace(domainInfo?.DistinguishedName)) { + var searchBase = context switch { + NamingContext.Configuration => $"CN=Configuration,{domainInfo.DistinguishedName}", + NamingContext.Schema => $"CN=Schema,CN=Configuration,{domainInfo.DistinguishedName}", + NamingContext.Default => domainInfo.DistinguishedName, + _ => throw new ArgumentOutOfRangeException() + }; + + return (true, searchBase); + } + + return (false, default); + } + + /// + /// Resolves a for the specified domain, preferring a controlled + /// LDAP path that honors the configured (server, port, SSL, + /// AuthType, signing, cert verification, credentials). + /// + /// + /// Resolution order: + /// + /// Static cache lookup keyed by (or when null). + /// Controlled LDAP resolution via the connection pool (see ). + /// Uncontrolled fallback via System.DirectoryServices.ActiveDirectory.Domain.GetDomain, + /// gated on (see ). + /// + /// Successful results from either path are cached for the lifetime of the process (until + /// is invoked). + /// + public async Task<(bool Success, DomainInfo DomainInfo)> GetDomainInfoAsync(string domainName) { + // Null domain names are stored under a per-instance sentinel to match the existing + // _domainCache null-key convention used by the legacy GetDomain overloads. + var cacheKey = domainName ?? _nullCacheKey; + if (_domainInfoCache.TryGetValue(cacheKey, out var cached)) { + return (true, cached); + } + + // Preferred path: every LDAP call made here flows through LdapConnectionPool and + // therefore honors every flag on LdapConfig. + var (controlledOk, controlledInfo) = await ResolveDomainInfoControlledAsync(domainName); + if (controlledOk) { + _domainInfoCache.TryAdd(cacheKey, controlledInfo); + return (true, controlledInfo); + } + + // Resolve the effective domain hint so the remaining tiers can still operate when the + // caller passed null (netonly / workgroup hosts rely on LdapConfig.CurrentUserDomain here). + var effectiveDomain = ResolveEffectiveDomainHint(domainName); + + // Secondary controlled path: bind directly to the domain name with a one-shot + // LdapConnection honoring LdapConfig. Tried before ADSI because this tier is the + // only one that can express fine-grained LdapConfig.AuthType and + // LdapConfig.DisableCertVerification - running ADSI first would silently ignore + // those flags whenever the ADSI bind happened to succeed. + if (!string.IsNullOrWhiteSpace(effectiveDomain)) { + var (directOk, directInfo) = + await TryResolveDomainInfoViaDirectLdapAsync(effectiveDomain, _ldapConfig, _log); + if (directOk) { + _domainInfoCache.TryAdd(cacheKey, directInfo); + return (true, directInfo); + } + } + + // Tertiary controlled path: ADSI. DirectoryEntry's serverless binding invokes + // DsGetDcName internally, so NetBIOS short names and domains without a direct DNS + // A record still bind here after the raw-LDAP tier above could not resolve them. + if (!string.IsNullOrWhiteSpace(effectiveDomain)) { + var (adsiOk, adsiInfo) = + await TryResolveDomainInfoViaDirectoryEntryAsync(effectiveDomain, _ldapConfig, _log); + if (adsiOk) { + _domainInfoCache.TryAdd(cacheKey, adsiInfo); + return (true, adsiInfo); + } + } + + // Opt-in fallback. TryGetDomainInfoViaUncontrolledFallback short-circuits when + // AllowFallbackToUncontrolledLdap is disabled, so when the flag is off this call is a no-op. + if (TryGetDomainInfoViaUncontrolledFallback(effectiveDomain, _ldapConfig, _log, out var fallbackInfo)) { + _domainInfoCache.TryAdd(cacheKey, fallbackInfo); + return (true, fallbackInfo); + } + + return (false, null); + } + + /// + /// Convenience overload that resolves a for the user's current + /// domain. Equivalent to calling with null. + /// + public Task<(bool Success, DomainInfo DomainInfo)> GetDomainInfoAsync() { + return GetDomainInfoAsync(null); + } + + /// + /// Static counterpart to intended for internal + /// consumers that cannot hold an instance (notably + /// and , which are + /// themselves pieces of the connection infrastructure and can't reenter it transparently). + /// + /// The target domain. Must be non-empty; this entry point does not resolve a default. + /// Config used to honor the gate and to build fallback credentials. + /// Logger used for debug-level diagnostics on failure paths. + /// + /// Results from both paths are stored in the same static + /// used by the instance overload, so a successful lookup here also benefits later + /// calls on the same process. + /// + internal static async Task<(bool Success, DomainInfo DomainInfo)> GetDomainInfoStaticAsync( + string domainName, LdapConfig config, ILogger log = null) { + // Unlike the instance overload, the static entry point does not fabricate a current-domain + // hint - its callers always know which domain they are asking about. + if (string.IsNullOrWhiteSpace(domainName)) { + return (false, null); + } + + if (_domainInfoCache.TryGetValue(domainName, out var cached)) { + return (true, cached); + } + + // Secondary controlled path: a one-shot LdapConnection bound directly to the domain + // name, honoring LdapConfig. Tried before ADSI because this tier is the only one + // that can express fine-grained LdapConfig.AuthType and LdapConfig.DisableCertVerification. + var (directOk, directInfo) = await TryResolveDomainInfoViaDirectLdapAsync(domainName, config, log); + if (directOk) { + _domainInfoCache.TryAdd(domainName, directInfo); + return (true, directInfo); + } + + // Tertiary controlled path: ADSI. DirectoryEntry's serverless binding handles + // NetBIOS short names and domains whose DNS layout does not expose an A record on + // the domain name itself, at the cost of not honoring every LdapConfig flag. + var (adsiOk, adsiInfo) = await TryResolveDomainInfoViaDirectoryEntryAsync(domainName, config, log); + if (adsiOk) { + _domainInfoCache.TryAdd(domainName, adsiInfo); + return (true, adsiInfo); + } + + if (TryGetDomainInfoViaUncontrolledFallback(domainName, config, log, out var fallbackInfo)) { + _domainInfoCache.TryAdd(domainName, fallbackInfo); + return (true, fallbackInfo); + } + + return (false, null); + } + + /// + /// Instance-level adapter over that supplies + /// this utils' connection pool and logger, and substitutes a current-domain hint when + /// is null/empty. + /// + /// + /// The hint is taken from when set, otherwise + /// from . The config override is the only workable + /// hint in netonly contexts (runas /netonly) and on workgroup machines, where the + /// OS API reports the local machine name rather than the target AD domain. If neither is + /// usable the connection pool will fail to resolve the hint and this method returns + /// (false, null) so the orchestrator can fall through to the uncontrolled fallback. + /// + private Task<(bool Success, DomainInfo DomainInfo)> ResolveDomainInfoControlledAsync(string domainName) { + var hint = ResolveEffectiveDomainHint(domainName); + if (string.IsNullOrWhiteSpace(hint)) { + return Task.FromResult<(bool, DomainInfo)>((false, null)); + } + + return ResolveDomainInfoControlledAsyncCore(hint, _connectionPool, _log); + } + + /// + /// Resolves the domain name to use when the caller did not supply one. Walks a five-step + /// preference order designed to maximize resolution success without paying an RPC on the + /// common path. + /// + /// + /// Order of preference: + /// + /// Explicit — caller-supplied; always wins. + /// — deterministic escape hatch for any + /// scenario where the OS-provided hint is wrong. Preferred over the uncontrolled tier below + /// because it's free and doesn't require opting into uncontrolled calls. + /// , only when it differs from + /// . The env var returning the machine name is a + /// reliable signal that the primary token carries no domain identity (netonly, workgroup, + /// LocalSystem with no stored creds). In every other case - normal interactive logons, + /// regular runas, service accounts in a joined machine's domain - the env var is + /// correct and this branch short-circuits the rest of the resolver at zero cost. + /// Uncontrolled DC-locator via Domain.GetDomain (see + /// ). Only fires when + /// is on. Issues + /// Domain.GetDomain(new DirectoryContext(Domain)) with no explicit credentials or + /// target, letting the SDS/DsGetDcName stack resolve the current user's domain from the + /// thread's outbound authentication context. In runas /netonly that context is the + /// alt credential (not the local primary token that step 3 failed on), so this recovers + /// the real target domain. Result cached process-wide after the first successful attempt. + /// Last-resort even when it equals the + /// machine name. Downstream tiers will almost certainly fail to bind against this, but + /// returning the env var here keeps behavior identical to the pre-change code for users who + /// haven't opted into the uncontrolled fallback. + /// + /// + internal string ResolveEffectiveDomainHint(string domainName) { + // 1. Explicit argument wins unconditionally. + if (!string.IsNullOrWhiteSpace(domainName)) + return domainName; + + // 2. Config-provided override. Cheap, deterministic, no network I/O. + if (!string.IsNullOrWhiteSpace(_ldapConfig?.CurrentUserDomain)) + return _ldapConfig.CurrentUserDomain; + + // 3. Env var, but only when it actually carries a domain identity. UserDomainName == + // MachineName is the canonical signal for "no domain on the primary token" and covers + // netonly, workgroup, and LocalSystem cases without false positives on domain-joined + // setups (where the two always differ). + var envDomain = Environment.UserDomainName; + if (!string.IsNullOrWhiteSpace(envDomain) && + !string.Equals(envDomain, Environment.MachineName, StringComparison.OrdinalIgnoreCase)) { + return envDomain; + } + + // 4. Opt-in uncontrolled DC-locator. Only pays the RPC when step 3 determined the env var + // is unusable AND the caller has explicitly allowed uncontrolled calls. No credentials + // are passed - the SDS stack uses the thread's outbound auth context, which in netonly + // is the alt credential rather than the local primary token. + if (TryResolveHintViaUncontrolledGetDomain(_ldapConfig, _log, out var credDomain)) { + return credDomain; + } + + // 5. Preserve pre-change behavior: return the env var even if it's the machine name. + // Lets downstream tiers decide how to fail. + return envDomain; + } + + /// + /// Uncontrolled hint resolver that invokes Domain.GetDomain(new DirectoryContext(Domain)) + /// with no explicit credentials or target name, letting the SDS/DsGetDcName stack resolve the + /// domain from the current thread's outbound authentication context. Gated behind + /// because it makes an unmanaged + /// DC-locator RPC that bypasses every other flag. + /// + /// + /// Complements step 3 in : that step consults + /// , which reads the process's primary token. In + /// runas /netonly the primary token is the local machine, so that value is useless; + /// however the LSA still attaches the alt credential to the thread for outbound network auth, + /// and DsGetDcName uses that context to locate a DC for the alt credential's actual + /// domain. Passing no username/password here is deliberate - + /// is not guaranteed to be a UPN or downlevel name, so + /// supplying it to could misdirect the locator; the + /// credential-less form instead lets Windows use whatever authentication context is already + /// bound to the thread. + /// + /// Result is cached process-wide in under + /// . The lock serializes the RPC so it fires at + /// most once per cycle even under concurrent callers. A stored empty + /// string is a "tried and failed" sentinel so repeat callers short-circuit without retrying. + /// + /// + private static bool TryResolveHintViaUncontrolledGetDomain( + LdapConfig config, ILogger log, out string domainName) { + domainName = null; + + // Hard gate: uncontrolled calls require explicit opt-in. + if (config == null || !config.AllowFallbackToUncontrolledLdap) return false; + + lock (_uncontrolledGetDomainHintLock) { + // Previously resolved or previously failed - either way, no new RPC. + if (_uncontrolledGetDomainHint != null) { + if (_uncontrolledGetDomainHint.Length == 0) return false; + domainName = _uncontrolledGetDomainHint; + return true; + } + + try { + // No name, no credentials: SDS resolves via the thread's outbound auth context, + // which is what surfaces the alt-creds domain under runas /netonly. + var ctx = new DirectoryContext(DirectoryContextType.Domain); + var domain = Domain.GetDomain(ctx); + var name = domain?.Name; + if (!string.IsNullOrEmpty(name)) { + _uncontrolledGetDomainHint = name; + domainName = name; + return true; + } + } + catch (Exception e) { + log?.LogDebug(e, + "TryResolveHintViaUncontrolledGetDomain: Domain.GetDomain failed"); + } + + // Negative-cache the failure so repeat cache-miss paths don't re-issue the RPC. + _uncontrolledGetDomainHint = string.Empty; + return false; + } + } + + /// + /// Core controlled-LDAP resolver. Builds a using only queries + /// that flow through the supplied , so every call + /// honors the attached to that pool. + /// + /// + /// Work performed, in order: + /// + /// Acquire a pooled connection to . The wrapper's cached rootDSE + /// supplies defaultNamingContext and configurationNamingContext without an extra round-trip. + /// Base search on the domain NC (objectClass=*) for objectSid (domain SID), + /// rootDomainNamingContext (forest root, used to compute ) + /// and fSMORoleOwner (PDC FSMO owner; stored as a DN to the NTDS Settings object). + /// If the PDC owner DN is resolved, a follow-up Base search on the owner's parent + /// server object retrieves dNSHostName to populate . + /// OneLevel search under CN=Partitions,<configNc> filtered by nCName + /// retrieves the legacy NetBIOS domain name from the matching crossRef object. + /// Subtree search on the domain NC for domain controllers + /// (userAccountControl:1.2.840.113556.1.4.803:=8192) collects DNS hostnames into + /// ; if the PDC lookup failed, the first DC is used as a fallback. + /// + /// All follow-up queries after the initial NC read are wrapped in try/catch and only + /// populate optional fields - a failure here still returns a partially-filled + /// with = false. A failure + /// to acquire the connection or to read the default NC is fatal and returns (false, null). + /// + private static async Task<(bool Success, DomainInfo DomainInfo)> ResolveDomainInfoControlledAsyncCore( + string domainName, ConnectionPoolManager pool, ILogger log) { + if (string.IsNullOrWhiteSpace(domainName) || pool == null) { + return (false, null); + } + + // Acquire a pooled connection to harvest rootDSE-derived naming contexts. + var (ok, wrapper, _) = await pool.GetLdapConnection(domainName, false); + if (!ok || wrapper == null) { + return (false, null); + } + + // GetSearchBase reads from the wrapper's cached rootDSE entry populated when the + // connection was established - no additional LDAP traffic is issued here. We release + // the connection immediately so subsequent Query(...) calls can reuse it from the pool. + string defaultNc; + string configNc; + try { + wrapper.GetSearchBase(NamingContext.Default, out defaultNc); + wrapper.GetSearchBase(NamingContext.Configuration, out configNc); + } + finally { + pool.ReleaseConnection(wrapper); + } + + if (string.IsNullOrWhiteSpace(defaultNc)) { + return (false, null); + } + + // Canonical name is always derivable from the default NC (e.g. DC=contoso,DC=local -> CONTOSO.LOCAL). + var info = new DomainInfo { + Name = Helpers.DistinguishedNameToDomain(defaultNc).ToUpper(), + DistinguishedName = defaultNc, + }; + + // Base search on the domain NC harvests the domain SID, forest root NC, and PDC FSMO owner DN + // in a single round-trip. + try { + var baseRes = await pool.Query(new LdapQueryParameters { + DomainName = domainName, + SearchBase = defaultNc, + SearchScope = SearchScope.Base, + LDAPFilter = "(objectClass=*)", + Attributes = new[] { + LDAPProperties.ObjectSID, + LDAPProperties.RootDomainNamingContext, + LDAPProperties.FSMORoleOwner, + }, + }).DefaultIfEmpty(LdapResult.Fail()).FirstOrDefaultAsync(); + + if (baseRes.IsSuccess) { + // objectSid on the domain NC itself is the domain SID (S-1-5-21-a-b-c). + if (baseRes.Value.TryGetSecurityIdentifier(out var sid) && !string.IsNullOrEmpty(sid)) { + info.DomainSid = sid.ToUpper(); + } + + // rootDomainNamingContext points at the forest root domain's NC even when queried + // against a child domain, giving us the forest name without a separate GC lookup. + if (baseRes.Value.TryGetProperty(LDAPProperties.RootDomainNamingContext, out var rootNc) && + !string.IsNullOrEmpty(rootNc)) { + info.ForestName = Helpers.DistinguishedNameToDomain(rootNc).ToUpper(); + } + + // fSMORoleOwner on the domain NC is a DN to the NTDS Settings object of the PDC, + // e.g. "CN=NTDS Settings,CN=DC01,CN=Servers,CN=Default-First-Site-Name,...". + // Strip the NTDS Settings RDN to get the Server object DN, then read its dNSHostName. + if (baseRes.Value.TryGetProperty(LDAPProperties.FSMORoleOwner, out var fsmoOwner) && + TryStripNtdsSettingsPrefix(fsmoOwner, out var serverDn)) { + var pdcRes = await pool.Query(new LdapQueryParameters { + DomainName = domainName, + SearchBase = serverDn, + SearchScope = SearchScope.Base, + LDAPFilter = "(objectClass=*)", + Attributes = new[] { LDAPProperties.DNSHostName }, + }).DefaultIfEmpty(LdapResult.Fail()).FirstOrDefaultAsync(); + + if (pdcRes.IsSuccess && + pdcRes.Value.TryGetProperty(LDAPProperties.DNSHostName, out var pdcName)) { + info.PrimaryDomainController = pdcName; + } + } + } + } + catch (Exception ex) { + log?.LogDebug(ex, "ResolveDomainInfoControlled: base query failed for {Domain}", domainName); + } + + // NetBIOS name lives on the crossRef entry whose nCName matches the domain NC. Cross-references + // live under CN=Partitions in the configuration NC, so this query only runs if we read the + // configuration NC above. + if (!string.IsNullOrWhiteSpace(configNc)) { try { - var entry = domainObj.GetDirectoryEntry().ToDirectoryObject(); - if (entry.TryGetProperty(property, out var searchBase)) { - return (true, searchBase); + var nbRes = await pool.Query(new LdapQueryParameters { + DomainName = domainName, + SearchBase = $"CN=Partitions,{configNc}", + SearchScope = SearchScope.OneLevel, + LDAPFilter = $"(&(objectClass=crossRef)({LDAPProperties.NCName}={defaultNc}))", + Attributes = new[] { LDAPProperties.NetbiosName }, + }).DefaultIfEmpty(LdapResult.Fail()).FirstOrDefaultAsync(); + + if (nbRes.IsSuccess && nbRes.Value.TryGetProperty(LDAPProperties.NetbiosName, out var nb)) { + info.NetBiosName = nb; + } + } + catch (Exception ex) { + log?.LogDebug(ex, "ResolveDomainInfoControlled: netbios lookup failed for {Domain}", domainName); + } + } + + // DC enumeration uses the canonical "DC object = computer with SERVER_TRUST_ACCOUNT in UAC" + // bit test (0x2000). This is the same filter used elsewhere in the project via CommonFilters. + try { + var dcs = new List(); + var dcEnum = pool.Query(new LdapQueryParameters { + DomainName = domainName, + SearchBase = defaultNc, + LDAPFilter = CommonFilters.DomainControllers, + Attributes = new[] { LDAPProperties.DNSHostName }, + }); + await foreach (var dcRes in dcEnum) { + if (dcRes.IsSuccess && + dcRes.Value.TryGetProperty(LDAPProperties.DNSHostName, out var dcName) && + !string.IsNullOrEmpty(dcName)) { + dcs.Add(dcName); + } + } + + info.DomainControllers = dcs; + // Last-resort PDC: if the FSMO resolution above failed, any DC is a reasonable + // fallback target for callers that just want a working DC name. + if (string.IsNullOrEmpty(info.PrimaryDomainController) && dcs.Count > 0) { + info.PrimaryDomainController = dcs[0]; + } + } + catch (Exception ex) { + log?.LogDebug(ex, "ResolveDomainInfoControlled: DC enumeration failed for {Domain}", domainName); + } + + return (true, info); + } + + /// + /// Strips the leading CN=NTDS Settings, RDN from a FSMO role owner DN, yielding the + /// DN of the parent Server object. AD stores FSMO ownership as a DN pointing at the NTDS + /// Settings object nested under the server, but the dNSHostName attribute lives on + /// the server one level up. + /// + /// True if the prefix was found and a non-empty parent DN was produced. + internal static bool TryStripNtdsSettingsPrefix(string fsmoRoleOwnerDn, out string serverDn) { + serverDn = null; + if (string.IsNullOrEmpty(fsmoRoleOwnerDn)) return false; + const string prefix = "CN=NTDS Settings,"; + if (!fsmoRoleOwnerDn.StartsWith(prefix, StringComparison.OrdinalIgnoreCase)) return false; + serverDn = fsmoRoleOwnerDn.Substring(prefix.Length); + return !string.IsNullOrEmpty(serverDn); + } + + /// + /// Uncontrolled fallback that populates a by calling + /// System.DirectoryServices.ActiveDirectory.Domain.GetDomain. + /// + /// + /// This path is intentionally opt-in: it returns false immediately unless + /// is set. When enabled, it mirrors + /// the exact construction used by the legacy + /// LdapUtils.GetDomain overloads so behavior is bit-identical to the pre-change code. + /// + /// The call into Domain.GetDomain does not honor any flag + /// beyond the username/password branches - server, port, SSL, signing, and cert-verification + /// settings are all bypassed because that API performs its own DC discovery via the native + /// DS RPC stack. The resulting is marked with + /// = true so callers and tests can detect the path. + /// + /// + /// Every optional property access (Forest, PdcRoleOwner, DomainControllers, + /// GetDirectoryEntry) is wrapped in its own try/catch because each of these can + /// blocking-dial the network or attempt delegated auth and may fail independently even + /// when the top-level Domain was obtained successfully. + /// + /// + private static bool TryGetDomainInfoViaUncontrolledFallback(string domainName, LdapConfig config, + ILogger log, out DomainInfo info) { + info = null; + // Hard gate - when the flag is off this method is a no-op regardless of what the + // surrounding LDAP config looks like. + if (config == null || !config.AllowFallbackToUncontrolledLdap) { + return false; + } + + try { + // Matches the DirectoryContext construction in the legacy GetDomain overloads so + // enabling this fallback yields identical results to the pre-change code path. + DirectoryContext context; + if (config.Username != null) + context = domainName != null + ? new DirectoryContext(DirectoryContextType.Domain, domainName, config.Username, + config.Password) + : new DirectoryContext(DirectoryContextType.Domain, config.Username, config.Password); + else + context = domainName != null + ? new DirectoryContext(DirectoryContextType.Domain, domainName) + : new DirectoryContext(DirectoryContextType.Domain); + + // Blocking External Call + var domain = Domain.GetDomain(context); + if (domain == null) { + return false; + } + + info = new DomainInfo { + Name = domain.Name?.ToUpper(), + DistinguishedName = !string.IsNullOrEmpty(domain.Name) + ? Helpers.DomainNameToDistinguishedName(domain.Name) + : null, + }; + + // Forest lookup triggers a separate bind under the hood; swallow any failure and + // leave ForestName null rather than losing the rest of the DomainInfo. + try { + info.ForestName = domain.Forest?.Name?.ToUpper(); + } + catch { + //pass + } + + // PdcRoleOwner.Name is the DNS hostname of the PDC. Separately guarded because + // it performs its own RPC lookup. + try { + info.PrimaryDomainController = domain.PdcRoleOwner?.Name; + } + catch { + //pass + } + + // DomainControllers enumeration discovers DCs via DsGetDcName; each property + // access on a returned controller can also fault independently. + try { + var dcs = new List(); + foreach (DomainController dc in domain.DomainControllers) { + try { + if (!string.IsNullOrEmpty(dc?.Name)) dcs.Add(dc.Name); + } + catch { + //pass + } + } + + info.DomainControllers = dcs; + } + catch { + //pass + } + + // GetDirectoryEntry binds to the domain NC via SDS and reads objectSid. Cheapest + // way to get the domain SID from an already-resolved Domain object without another + // DirectoryContext round-trip. The raw DirectoryEntry inherits only the + // DirectoryContext credentials, so apply the LdapConfig transport/auth flags + // (matching Helpers.CreateDirectoryEntry) before the first property access forces + // the bind. + try { + var rawEntry = domain.GetDirectoryEntry(); + var authType = AuthenticationTypes.Secure; + if (config.ForceSSL) { + authType |= AuthenticationTypes.SecureSocketsLayer; + } + if (!config.DisableSigning && !config.ForceSSL) { + authType |= AuthenticationTypes.Signing | AuthenticationTypes.Sealing; + } + rawEntry.AuthenticationType = authType; + if (config.Username != null) { + rawEntry.Username = config.Username; + rawEntry.Password = config.Password; + } + + var entry = rawEntry.ToDirectoryObject(); + if (entry.TryGetSecurityIdentifier(out var sid) && !string.IsNullOrEmpty(sid)) { + info.DomainSid = sid.ToUpper(); } } catch { //pass } - var name = domainObj.Name; - if (!string.IsNullOrWhiteSpace(name)) { - var tempPath = Helpers.DomainNameToDistinguishedName(name); + return true; + } + catch (Exception e) { + log?.LogDebug(e, "TryGetDomainInfoViaUncontrolledFallback failed for domain {Name}", domainName); + return false; + } + } - var searchBase = context switch { - NamingContext.Configuration => $"CN=Configuration,{tempPath}", - NamingContext.Schema => $"CN=Schema,CN=Configuration,{tempPath}", - NamingContext.Default => tempPath, - _ => throw new ArgumentOutOfRangeException() + /// + /// Controlled resolver that uses ADSI () + /// via . Sits after the one-shot + /// path in and + /// , acting as a DC-locator-aware fallback for the + /// cases the raw-LDAP tier cannot resolve on its own. + /// + /// + /// ADSI's serverless binding transparently invokes DsGetDcName under the hood, so + /// NetBIOS short names and environments where has no direct + /// DNS A record still bind here after the preceding one-shot + /// tier could not locate a DC. ADSI honors , + /// , , + /// , and . It + /// cannot honor (no ADSI API) or + /// fine-grained selection (always Negotiate via + /// AuthenticationTypes.Secure), which is precisely why this tier is ordered *after* + /// the one-shot path: callers that configured those flags will have them respected by the + /// raw-LDAP tier and only reach ADSI if that tier failed outright. + /// + /// Populates , , + /// , , and + /// . Does not populate + /// or ; + /// those are only populated by the pool-based tier, which enumerates via subtree search. + /// + /// + /// ADSI property access is synchronous and blocking; the body is offloaded to a + /// thread-pool task so callers remain non-blocking. + /// + /// + private static async Task<(bool Success, DomainInfo DomainInfo)> TryResolveDomainInfoViaDirectoryEntryAsync( + string domainName, LdapConfig config, ILogger log) { + if (string.IsNullOrWhiteSpace(domainName) || config == null) { + return (false, null); + } + + return await Task.Run<(bool Success, DomainInfo DomainInfo)>(() => { + DomainInfo info; + IDirectoryObject root; + try { + root = Helpers.CreateDirectoryEntry($"LDAP://{domainName}", config); + + // Force the bind by reading the DN. When the bind fails (unreachable DC, auth + // failure, etc.) TryGetDistinguishedName swallows the underlying COMException + // and returns false. + if (!root.TryGetDistinguishedName(out var defaultNc) || + string.IsNullOrWhiteSpace(defaultNc)) { + return (false, null); + } + + info = new DomainInfo { + Name = Helpers.DistinguishedNameToDomain(defaultNc).ToUpper(), + DistinguishedName = defaultNc, }; - return (true, searchBase); + if (root.TryGetSecurityIdentifier(out var sid) && !string.IsNullOrEmpty(sid)) { + info.DomainSid = sid.ToUpper(); + } + } + catch (Exception e) { + log?.LogDebug(e, "DirectoryEntry tier: base bind failed for {Domain}", domainName); + return (false, null); + } + + // RootDSE on the same bound server yields the forest root NC without a separate GC bind. + try { + var rootDse = Helpers.CreateDirectoryEntry($"LDAP://{domainName}/RootDSE", config); + if (rootDse.TryGetProperty(LDAPProperties.RootDomainNamingContext, out var rootNc) && + !string.IsNullOrEmpty(rootNc)) { + info.ForestName = Helpers.DistinguishedNameToDomain(rootNc).ToUpper(); + } + } + catch (Exception ex) { + log?.LogDebug(ex, + "DirectoryEntry tier: RootDSE read failed for {Domain}", domainName); + } + + // fSMORoleOwner on the domain NC is the PDC NTDS Settings DN; its parent server + // object carries dNSHostName. Mirrors the extraction logic in the other tiers. + try { + if (root.TryGetProperty(LDAPProperties.FSMORoleOwner, out var fsmoOwner) && + TryStripNtdsSettingsPrefix(fsmoOwner, out var serverDn)) { + var server = Helpers.CreateDirectoryEntry($"LDAP://{serverDn}", config); + if (server.TryGetProperty(LDAPProperties.DNSHostName, out var pdc) && + !string.IsNullOrEmpty(pdc)) { + info.PrimaryDomainController = pdc; + } + } + } + catch (Exception ex) { + log?.LogDebug(ex, + "DirectoryEntry tier: PDC lookup failed for {Domain}", domainName); + } + + return (true, info); + }).ConfigureAwait(false); + } + + /// + /// Constructs and binds a one-shot + /// to honoring every option on (port, + /// SSL, signing, auth type, credentials, cert verification). This is the same configuration + /// shape used by LdapConnectionPool.CreateBaseConnection, duplicated here because + /// this code runs *outside* any pool and must not take a dependency on pool internals. + /// + /// + /// Tries SSL first then plain LDAP, respecting : when + /// ForceSSL is set, only the SSL attempt is made. Returns null if neither transport + /// binds. The returned connection is owned by the caller and must be disposed. + /// + private static LdapConnection TryBindOneShotLdapConnection( + string target, LdapConfig config, ILogger log) { + var transports = config.ForceSSL ? new[] { true } : new[] { true, false }; + foreach (var ssl in transports) { + LdapConnection connection = null; + try { + var port = config.GetPort(ssl); + var identifier = new LdapDirectoryIdentifier( + target, port, false, false); + connection = new LdapConnection(identifier) { + Timeout = TimeSpan.FromSeconds(30), + }; + connection.SessionOptions.ProtocolVersion = 3; + connection.SessionOptions.ReferralChasing = + ReferralChasingOptions.None; + if (ssl) connection.SessionOptions.SecureSocketLayer = true; + + if (config.DisableSigning || ssl) { + connection.SessionOptions.Signing = false; + connection.SessionOptions.Sealing = false; + } else { + connection.SessionOptions.Signing = true; + connection.SessionOptions.Sealing = true; + } + + if (config.DisableCertVerification) + connection.SessionOptions.VerifyServerCertificate = (_, __) => true; + + if (config.Username != null) { + connection.Credential = new NetworkCredential(config.Username, config.Password); + } + + connection.AuthType = config.AuthType; + connection.Bind(); + return connection; + } + catch (Exception e) { + log?.LogDebug(e, + "TryBindOneShotLdapConnection: bind failed for {Target} ssl={Ssl}", target, ssl); + connection?.Dispose(); } } + return null; + } - return (false, default); + /// + /// Controlled resolver used when no is available to + /// route queries through. Binds a one-shot + /// directly to (treating it as a DNS domain name that resolves + /// to a DC via standard round-robin A/SRV records) and populates a + /// by issuing the same rootDSE + Base + Subtree queries as + /// , just over a private connection. + /// + /// + /// Intended as the intermediate step in between the + /// pool-based controlled path and the uncontrolled Domain.GetDomain fallback, so that + /// callers inside (which must pass pool = null to + /// avoid reentering connection acquisition) still have a controlled resolution option when + /// is off. The connection is torn + /// down before returning; no state is added to any pool. + /// + private static async Task<(bool Success, DomainInfo DomainInfo)> TryResolveDomainInfoViaDirectLdapAsync( + string domainName, LdapConfig config, ILogger log) { + if (string.IsNullOrWhiteSpace(domainName) || config == null) { + return (false, null); + } + + var connection = TryBindOneShotLdapConnection(domainName, config, log); + if (connection == null) { + return (false, null); + } + + // Offload the synchronous SendRequest calls so we don't block the calling thread when + // called from an async code path (e.g. LdapConnectionPool.CreateLdapConnection). + return await Task.Run(() => { + try { + return ResolveDomainInfoFromConnection(connection, domainName, log); + } + finally { + connection.Dispose(); + } + }).ConfigureAwait(false); + } + + /// + /// Synchronous body of . Uses + /// SendRequest directly rather than the pool's Query because no pool is + /// available on this path. Follows the same attribute shape as + /// : rootDSE → domain NC base → + /// PDC server DN → DC subtree enumeration, each wrapped independently so a partial + /// failure still returns a usable . + /// + private static (bool Success, DomainInfo DomainInfo) ResolveDomainInfoFromConnection( + LdapConnection connection, string domainName, ILogger log) { + // 1. RootDSE - the authoritative source for the default/config/root NCs of the DC we bound to. + string defaultNc = null, configNc = null, rootNc = null; + try { + var rootReq = new SearchRequest( + "", "(objectClass=*)", SearchScope.Base, + new[] { + LDAPProperties.DefaultNamingContext, + LDAPProperties.ConfigurationNamingContext, + LDAPProperties.RootDomainNamingContext, + }); + var rootResp = (SearchResponse)connection.SendRequest(rootReq); + if (rootResp?.Entries != null && rootResp.Entries.Count > 0) { + var entry = new SearchResultEntryWrapper(rootResp.Entries[0]); + entry.TryGetProperty(LDAPProperties.DefaultNamingContext, out defaultNc); + entry.TryGetProperty(LDAPProperties.ConfigurationNamingContext, out configNc); + entry.TryGetProperty(LDAPProperties.RootDomainNamingContext, out rootNc); + } + } + catch (Exception e) { + log?.LogDebug(e, "Direct LDAP rootDSE read failed for {Domain}", domainName); + return (false, null); + } + + if (string.IsNullOrWhiteSpace(defaultNc)) { + return (false, null); + } + + var info = new DomainInfo { + Name = Helpers.DistinguishedNameToDomain(defaultNc).ToUpper(), + DistinguishedName = defaultNc, + }; + if (!string.IsNullOrWhiteSpace(rootNc)) { + info.ForestName = Helpers.DistinguishedNameToDomain(rootNc).ToUpper(); + } + + // 2. Domain NC base search - objectSid + fsmoRoleOwner in one round-trip. + string fsmoOwner = null; + try { + var domReq = new SearchRequest( + defaultNc, "(objectClass=*)", SearchScope.Base, + new[] { LDAPProperties.ObjectSID, LDAPProperties.FSMORoleOwner }); + var domResp = (SearchResponse)connection.SendRequest(domReq); + if (domResp?.Entries != null && domResp.Entries.Count > 0) { + var entry = new SearchResultEntryWrapper(domResp.Entries[0]); + if (entry.TryGetSecurityIdentifier(out var sid) && !string.IsNullOrEmpty(sid)) { + info.DomainSid = sid.ToUpper(); + } + entry.TryGetProperty(LDAPProperties.FSMORoleOwner, out fsmoOwner); + } + } + catch (Exception e) { + log?.LogDebug(e, "Direct LDAP domain NC read failed for {Domain}", domainName); + } + + // 3. PDC server dNSHostName via the fsmoRoleOwner DN parent. + if (TryStripNtdsSettingsPrefix(fsmoOwner, out var serverDn)) { + try { + var pdcReq = new SearchRequest( + serverDn, "(objectClass=*)", SearchScope.Base, + new[] { LDAPProperties.DNSHostName }); + var pdcResp = (SearchResponse)connection.SendRequest(pdcReq); + if (pdcResp?.Entries != null && pdcResp.Entries.Count > 0) { + var entry = new SearchResultEntryWrapper(pdcResp.Entries[0]); + if (entry.TryGetProperty(LDAPProperties.DNSHostName, out var pdcName)) { + info.PrimaryDomainController = pdcName; + } + } + } + catch (Exception e) { + log?.LogDebug(e, "Direct LDAP PDC lookup failed for {Domain}", domainName); + } + } + + // 4. Domain controller enumeration - same filter as CommonFilters.DomainControllers. + try { + var dcs = new List(); + var dcReq = new SearchRequest( + defaultNc, CommonFilters.DomainControllers, SearchScope.Subtree, + new[] { LDAPProperties.DNSHostName }); + var dcResp = (SearchResponse)connection.SendRequest(dcReq); + if (dcResp?.Entries != null) { + foreach (SearchResultEntry e in dcResp.Entries) { + var wrap = new SearchResultEntryWrapper(e); + if (wrap.TryGetProperty(LDAPProperties.DNSHostName, out var dcName) && + !string.IsNullOrEmpty(dcName)) { + dcs.Add(dcName); + } + } + } + info.DomainControllers = dcs; + if (string.IsNullOrEmpty(info.PrimaryDomainController) && dcs.Count > 0) { + info.PrimaryDomainController = dcs[0]; + } + } + catch (Exception e) { + log?.LogDebug(e, "Direct LDAP DC enumeration failed for {Domain}", domainName); + } + + return (true, info); } public void ResetUtils() { _unresolvablePrincipals = new ConcurrentHashSet(StringComparer.OrdinalIgnoreCase); _domainCache = new ConcurrentDictionary(); + _domainInfoCache = new ConcurrentDictionary(StringComparer.OrdinalIgnoreCase); _domainControllers = new ConcurrentHashSet(StringComparer.OrdinalIgnoreCase); + lock (_uncontrolledGetDomainHintLock) { + _uncontrolledGetDomainHint = null; + } _connectionPool?.Dispose(); _connectionPool = new ConnectionPoolManager(_ldapConfig, scanner: _portScanner); - + // Metrics LdapMetrics.ResetInFlight(); } diff --git a/src/CommonLib/Processors/GPOLocalGroupProcessor.cs b/src/CommonLib/Processors/GPOLocalGroupProcessor.cs index 28a6996f6..2f1212ca4 100644 --- a/src/CommonLib/Processors/GPOLocalGroupProcessor.cs +++ b/src/CommonLib/Processors/GPOLocalGroupProcessor.cs @@ -66,13 +66,14 @@ public async Task ReadGPOLocalGroups(string gpLink, string string domain; //If our dn is null, use our default domain if (string.IsNullOrEmpty(distinguishedName)) { - if (!_utils.GetDomain(out var domainResult)) { + var (ok, info) = await _utils.GetDomainInfoAsync(); + if (!ok || string.IsNullOrEmpty(info?.Name)) { return ret; } - domain = domainResult.Name; + domain = info.Name; } else { - domain = Helpers.DistinguishedNameToDomain(distinguishedName); + domain = Helpers.DistinguishedNameToDomain(distinguishedName); } // First lets check if this OU actually has computers that it contains. If not, then we'll ignore it. diff --git a/test/unit/Facades/MockLdapUtils.cs b/test/unit/Facades/MockLdapUtils.cs index 1e5f1194d..9946bd0f2 100644 --- a/test/unit/Facades/MockLdapUtils.cs +++ b/test/unit/Facades/MockLdapUtils.cs @@ -703,6 +703,14 @@ public bool GetDomain(out Domain domain) { return false; } + public Task<(bool Success, DomainInfo DomainInfo)> GetDomainInfoAsync(string domainName) { + return Task.FromResult<(bool, DomainInfo)>((false, null)); + } + + public Task<(bool Success, DomainInfo DomainInfo)> GetDomainInfoAsync() { + return Task.FromResult<(bool, DomainInfo)>((false, null)); + } + public async Task<(bool Success, TypedPrincipal Principal)> ResolveAccountName(string name, string domain) { var res = name.ToUpper() switch { "ADMINISTRATOR" => new TypedPrincipal( diff --git a/test/unit/GPOLocalGroupProcessorTest.cs b/test/unit/GPOLocalGroupProcessorTest.cs index 80f3a59a7..8dedb0dd4 100644 --- a/test/unit/GPOLocalGroupProcessorTest.cs +++ b/test/unit/GPOLocalGroupProcessorTest.cs @@ -323,8 +323,8 @@ public async Task GPOLocalGroupProcessor_ReadGPOLocalGroups() { .Returns(mockComputerResults.ToAsyncEnumerable) .Returns(mockGCPFileSysPathResults.ToAsyncEnumerable) .Returns(Array.Empty>().ToAsyncEnumerable); - var domain = MockableDomain.Construct("TESTLAB.LOCAL"); - mockLDAPUtils.Setup(x => x.GetDomain(out domain)).Returns(true); + var domainInfo = new DomainInfo { Name = "TESTLAB.LOCAL" }; + mockLDAPUtils.Setup(x => x.GetDomainInfoAsync()).ReturnsAsync((true, domainInfo)); var processor = new GPOLocalGroupProcessor(mockLDAPUtils.Object); diff --git a/test/unit/LDAPUtilsTest.cs b/test/unit/LDAPUtilsTest.cs index 149f52449..6f9966f15 100644 --- a/test/unit/LDAPUtilsTest.cs +++ b/test/unit/LDAPUtilsTest.cs @@ -647,5 +647,153 @@ public async Task EnterpriseDomainControllersGroup_CorrectValues() { Assert.Equal("TESTLAB.LOCAL-S-1-5-9", entDCGroup.ObjectIdentifier); Assert.Equal(3, entDCGroup.Members.Length); } + + // --------------------------------------------------------------------------- + // AllowFallbackToUncontrolledLdap gate and DomainInfo resolution + // --------------------------------------------------------------------------- + + [Fact] + public void LdapConfig_AllowFallbackToUncontrolledLdap_DefaultsToFalse() { + var config = new LdapConfig(); + Assert.False(config.AllowFallbackToUncontrolledLdap); + } + + [Fact] + public void LdapConfig_ToString_IncludesAllowFallbackFlag() { + var offConfig = new LdapConfig(); + Assert.Contains("AllowFallbackToUncontrolledLdap: False", offConfig.ToString()); + + var onConfig = new LdapConfig { AllowFallbackToUncontrolledLdap = true }; + Assert.Contains("AllowFallbackToUncontrolledLdap: True", onConfig.ToString()); + } + + [Fact] + public void LdapConfig_CurrentUserDomain_DefaultsToNull() { + var config = new LdapConfig(); + Assert.Null(config.CurrentUserDomain); + } + + [Fact] + public void LdapConfig_ToString_IncludesCurrentUserDomain_WhenSet() { + var unset = new LdapConfig(); + Assert.DoesNotContain("CurrentUserDomain:", unset.ToString()); + + var set = new LdapConfig { CurrentUserDomain = "CONTOSO.LOCAL" }; + Assert.Contains("CurrentUserDomain: CONTOSO.LOCAL", set.ToString()); + } + + [Fact] + public async Task GetDomainInfoAsync_ControlledPathFails_FallbackDisabled_ReturnsFailure() { + var utils = new LdapUtils(); + utils.SetLdapConfig(new LdapConfig { + Server = "unreachable.invalid.test", + AllowFallbackToUncontrolledLdap = false + }); + + var (success, info) = await utils.GetDomainInfoAsync("unreachable.invalid.test"); + Assert.False(success); + Assert.Null(info); + } + + [Fact] + public void GetDomain_OutDomain_ReturnsFalse_WhenFallbackDisabled() { + var utils = new LdapUtils(); + utils.SetLdapConfig(new LdapConfig { AllowFallbackToUncontrolledLdap = false }); + + Assert.False(utils.GetDomain(out var currentDomain)); + Assert.Null(currentDomain); + + Assert.False(utils.GetDomain("unreachable.invalid.test", out var namedDomain)); + Assert.Null(namedDomain); + + Assert.False(LdapUtils.GetDomain("unreachable.invalid.test", + new LdapConfig { AllowFallbackToUncontrolledLdap = false }, out var staticDomain)); + Assert.Null(staticDomain); + } + + // --------------------------------------------------------------------------- + // TryStripNtdsSettingsPrefix + // --------------------------------------------------------------------------- + + [Fact] + public void TryStripNtdsSettingsPrefix_StandardDn_StripsPrefixAndReturnsServerDn() { + const string input = + "CN=NTDS Settings,CN=DC01,CN=Servers,CN=Default-First-Site-Name,CN=Sites,CN=Configuration,DC=contoso,DC=local"; + var ok = LdapUtils.TryStripNtdsSettingsPrefix(input, out var serverDn); + Assert.True(ok); + Assert.Equal( + "CN=DC01,CN=Servers,CN=Default-First-Site-Name,CN=Sites,CN=Configuration,DC=contoso,DC=local", + serverDn); + } + + [Fact] + public void TryStripNtdsSettingsPrefix_LowercasePrefix_StripsCaseInsensitively() { + const string input = "cn=ntds settings,CN=DC01,CN=Servers,DC=contoso,DC=local"; + var ok = LdapUtils.TryStripNtdsSettingsPrefix(input, out var serverDn); + Assert.True(ok); + Assert.Equal("CN=DC01,CN=Servers,DC=contoso,DC=local", serverDn); + } + + [Theory] + [InlineData(null)] + [InlineData("")] + public void TryStripNtdsSettingsPrefix_NullOrEmptyInput_ReturnsFalse(string input) { + var ok = LdapUtils.TryStripNtdsSettingsPrefix(input, out var serverDn); + Assert.False(ok); + Assert.Null(serverDn); + } + + [Fact] + public void TryStripNtdsSettingsPrefix_MissingPrefix_ReturnsFalse() { + const string input = "CN=DC01,CN=Servers,DC=contoso,DC=local"; + var ok = LdapUtils.TryStripNtdsSettingsPrefix(input, out var serverDn); + Assert.False(ok); + Assert.Null(serverDn); + } + + [Fact] + public void TryStripNtdsSettingsPrefix_PrefixOnly_ReturnsFalse() { + const string input = "CN=NTDS Settings,"; + var ok = LdapUtils.TryStripNtdsSettingsPrefix(input, out var serverDn); + Assert.False(ok); + Assert.Equal(string.Empty, serverDn); + } + + // --------------------------------------------------------------------------- + // ResolveEffectiveDomainHint + // --------------------------------------------------------------------------- + + [Fact] + public void ResolveEffectiveDomainHint_ExplicitDomain_WinsOverCurrentUserDomain() { + var utils = new LdapUtils(); + utils.SetLdapConfig(new LdapConfig { CurrentUserDomain = "FALLBACK.LOCAL" }); + + Assert.Equal("EXPLICIT.LOCAL", utils.ResolveEffectiveDomainHint("EXPLICIT.LOCAL")); + } + + [Theory] + [InlineData(null)] + [InlineData("")] + [InlineData(" ")] + public void ResolveEffectiveDomainHint_NullOrWhitespaceInput_FallsBackToCurrentUserDomain(string input) { + var utils = new LdapUtils(); + utils.SetLdapConfig(new LdapConfig { CurrentUserDomain = "CONTOSO.LOCAL" }); + + Assert.Equal("CONTOSO.LOCAL", utils.ResolveEffectiveDomainHint(input)); + } + + [Fact] + public void ResolveEffectiveDomainHint_WhitespaceCurrentUserDomain_FallsThroughToEnvironment() { + var utils = new LdapUtils(); + utils.SetLdapConfig(new LdapConfig { CurrentUserDomain = " " }); + + // Cannot assert a literal without pinning Environment.UserDomainName; asserting + // that the whitespace CurrentUserDomain was skipped and something else was chosen + // is sufficient to cover the branch. + var result = utils.ResolveEffectiveDomainHint(null); + Assert.False(string.IsNullOrWhiteSpace(result)); + Assert.NotEqual(" ", result); + } + } } \ No newline at end of file From b78b9398bdfdd7a4d7054cbaf905f2fd2568cce6 Mon Sep 17 00:00:00 2001 From: rvazarkar Date: Thu, 23 Apr 2026 19:12:38 -0400 Subject: [PATCH 02/20] fix: make DomainInfo immutable to prevent references being changed out from under us --- src/CommonLib/DomainInfo.cs | 31 +++++-- src/CommonLib/LdapUtils.cs | 116 +++++++++++++++--------- test/unit/GPOLocalGroupProcessorTest.cs | 2 +- 3 files changed, 99 insertions(+), 50 deletions(-) diff --git a/src/CommonLib/DomainInfo.cs b/src/CommonLib/DomainInfo.cs index adaa1196c..493b0e743 100644 --- a/src/CommonLib/DomainInfo.cs +++ b/src/CommonLib/DomainInfo.cs @@ -12,24 +12,41 @@ namespace SharpHoundCommonLib public sealed class DomainInfo { /// Upper-cased DNS name of the domain (e.g. CONTOSO.LOCAL). - public string Name { get; set; } + public string Name { get; } /// Default naming context distinguished name (e.g. DC=contoso,DC=local). - public string DistinguishedName { get; set; } + public string DistinguishedName { get; } /// Upper-cased DNS name of the forest root domain, when known. - public string ForestName { get; set; } + public string ForestName { get; } /// Domain SID (S-1-5-21-...) if resolved, otherwise null. - public string DomainSid { get; set; } + public string DomainSid { get; } /// Legacy NetBIOS domain name if resolved from the Partitions container, otherwise null. - public string NetBiosName { get; set; } + public string NetBiosName { get; } /// DNS hostname of the PDC FSMO role owner if resolved, otherwise null. - public string PrimaryDomainController { get; set; } + public string PrimaryDomainController { get; } /// DNS hostnames of known domain controllers for this domain. - public IReadOnlyList DomainControllers { get; set; } = Array.Empty(); + public IReadOnlyList DomainControllers { get; } + + public DomainInfo( + string name = null, + string distinguishedName = null, + string forestName = null, + string domainSid = null, + string netBiosName = null, + string primaryDomainController = null, + IReadOnlyList domainControllers = null) { + Name = name; + DistinguishedName = distinguishedName; + ForestName = forestName; + DomainSid = domainSid; + NetBiosName = netBiosName; + PrimaryDomainController = primaryDomainController; + DomainControllers = domainControllers ?? Array.Empty(); + } } } diff --git a/src/CommonLib/LdapUtils.cs b/src/CommonLib/LdapUtils.cs index d1272471d..0c4d2c345 100644 --- a/src/CommonLib/LdapUtils.cs +++ b/src/CommonLib/LdapUtils.cs @@ -1486,10 +1486,12 @@ private static bool TryResolveHintViaUncontrolledGetDomain( } // Canonical name is always derivable from the default NC (e.g. DC=contoso,DC=local -> CONTOSO.LOCAL). - var info = new DomainInfo { - Name = Helpers.DistinguishedNameToDomain(defaultNc).ToUpper(), - DistinguishedName = defaultNc, - }; + var name = Helpers.DistinguishedNameToDomain(defaultNc).ToUpper(); + string domainSid = null; + string forestName = null; + string primaryDomainController = null; + string netBiosName = null; + IReadOnlyList domainControllers = null; // Base search on the domain NC harvests the domain SID, forest root NC, and PDC FSMO owner DN // in a single round-trip. @@ -1509,14 +1511,14 @@ private static bool TryResolveHintViaUncontrolledGetDomain( if (baseRes.IsSuccess) { // objectSid on the domain NC itself is the domain SID (S-1-5-21-a-b-c). if (baseRes.Value.TryGetSecurityIdentifier(out var sid) && !string.IsNullOrEmpty(sid)) { - info.DomainSid = sid.ToUpper(); + domainSid = sid.ToUpper(); } // rootDomainNamingContext points at the forest root domain's NC even when queried // against a child domain, giving us the forest name without a separate GC lookup. if (baseRes.Value.TryGetProperty(LDAPProperties.RootDomainNamingContext, out var rootNc) && !string.IsNullOrEmpty(rootNc)) { - info.ForestName = Helpers.DistinguishedNameToDomain(rootNc).ToUpper(); + forestName = Helpers.DistinguishedNameToDomain(rootNc).ToUpper(); } // fSMORoleOwner on the domain NC is a DN to the NTDS Settings object of the PDC, @@ -1534,7 +1536,7 @@ private static bool TryResolveHintViaUncontrolledGetDomain( if (pdcRes.IsSuccess && pdcRes.Value.TryGetProperty(LDAPProperties.DNSHostName, out var pdcName)) { - info.PrimaryDomainController = pdcName; + primaryDomainController = pdcName; } } } @@ -1557,7 +1559,7 @@ private static bool TryResolveHintViaUncontrolledGetDomain( }).DefaultIfEmpty(LdapResult.Fail()).FirstOrDefaultAsync(); if (nbRes.IsSuccess && nbRes.Value.TryGetProperty(LDAPProperties.NetbiosName, out var nb)) { - info.NetBiosName = nb; + netBiosName = nb; } } catch (Exception ex) { @@ -1583,18 +1585,25 @@ private static bool TryResolveHintViaUncontrolledGetDomain( } } - info.DomainControllers = dcs; + domainControllers = dcs; // Last-resort PDC: if the FSMO resolution above failed, any DC is a reasonable // fallback target for callers that just want a working DC name. - if (string.IsNullOrEmpty(info.PrimaryDomainController) && dcs.Count > 0) { - info.PrimaryDomainController = dcs[0]; + if (string.IsNullOrEmpty(primaryDomainController) && dcs.Count > 0) { + primaryDomainController = dcs[0]; } } catch (Exception ex) { log?.LogDebug(ex, "ResolveDomainInfoControlled: DC enumeration failed for {Domain}", domainName); } - return (true, info); + return (true, new DomainInfo( + name: name, + distinguishedName: defaultNc, + forestName: forestName, + domainSid: domainSid, + netBiosName: netBiosName, + primaryDomainController: primaryDomainController, + domainControllers: domainControllers)); } /// @@ -1665,17 +1674,19 @@ private static bool TryGetDomainInfoViaUncontrolledFallback(string domainName, L return false; } - info = new DomainInfo { - Name = domain.Name?.ToUpper(), - DistinguishedName = !string.IsNullOrEmpty(domain.Name) - ? Helpers.DomainNameToDistinguishedName(domain.Name) - : null, - }; + var name = domain.Name?.ToUpper(); + var distinguishedName = !string.IsNullOrEmpty(domain.Name) + ? Helpers.DomainNameToDistinguishedName(domain.Name) + : null; + string forestName = null; + string primaryDomainController = null; + string domainSid = null; + IReadOnlyList domainControllers = null; // Forest lookup triggers a separate bind under the hood; swallow any failure and // leave ForestName null rather than losing the rest of the DomainInfo. try { - info.ForestName = domain.Forest?.Name?.ToUpper(); + forestName = domain.Forest?.Name?.ToUpper(); } catch { //pass @@ -1684,7 +1695,7 @@ private static bool TryGetDomainInfoViaUncontrolledFallback(string domainName, L // PdcRoleOwner.Name is the DNS hostname of the PDC. Separately guarded because // it performs its own RPC lookup. try { - info.PrimaryDomainController = domain.PdcRoleOwner?.Name; + primaryDomainController = domain.PdcRoleOwner?.Name; } catch { //pass @@ -1703,7 +1714,7 @@ private static bool TryGetDomainInfoViaUncontrolledFallback(string domainName, L } } - info.DomainControllers = dcs; + domainControllers = dcs; } catch { //pass @@ -1732,13 +1743,20 @@ private static bool TryGetDomainInfoViaUncontrolledFallback(string domainName, L var entry = rawEntry.ToDirectoryObject(); if (entry.TryGetSecurityIdentifier(out var sid) && !string.IsNullOrEmpty(sid)) { - info.DomainSid = sid.ToUpper(); + domainSid = sid.ToUpper(); } } catch { //pass } + info = new DomainInfo( + name: name, + distinguishedName: distinguishedName, + forestName: forestName, + domainSid: domainSid, + primaryDomainController: primaryDomainController, + domainControllers: domainControllers); return true; } catch (Exception e) { @@ -1785,7 +1803,11 @@ private static bool TryGetDomainInfoViaUncontrolledFallback(string domainName, L } return await Task.Run<(bool Success, DomainInfo DomainInfo)>(() => { - DomainInfo info; + string name; + string distinguishedName; + string domainSid = null; + string forestName = null; + string primaryDomainController = null; IDirectoryObject root; try { root = Helpers.CreateDirectoryEntry($"LDAP://{domainName}", config); @@ -1798,13 +1820,11 @@ private static bool TryGetDomainInfoViaUncontrolledFallback(string domainName, L return (false, null); } - info = new DomainInfo { - Name = Helpers.DistinguishedNameToDomain(defaultNc).ToUpper(), - DistinguishedName = defaultNc, - }; + name = Helpers.DistinguishedNameToDomain(defaultNc).ToUpper(); + distinguishedName = defaultNc; if (root.TryGetSecurityIdentifier(out var sid) && !string.IsNullOrEmpty(sid)) { - info.DomainSid = sid.ToUpper(); + domainSid = sid.ToUpper(); } } catch (Exception e) { @@ -1817,7 +1837,7 @@ private static bool TryGetDomainInfoViaUncontrolledFallback(string domainName, L var rootDse = Helpers.CreateDirectoryEntry($"LDAP://{domainName}/RootDSE", config); if (rootDse.TryGetProperty(LDAPProperties.RootDomainNamingContext, out var rootNc) && !string.IsNullOrEmpty(rootNc)) { - info.ForestName = Helpers.DistinguishedNameToDomain(rootNc).ToUpper(); + forestName = Helpers.DistinguishedNameToDomain(rootNc).ToUpper(); } } catch (Exception ex) { @@ -1833,7 +1853,7 @@ private static bool TryGetDomainInfoViaUncontrolledFallback(string domainName, L var server = Helpers.CreateDirectoryEntry($"LDAP://{serverDn}", config); if (server.TryGetProperty(LDAPProperties.DNSHostName, out var pdc) && !string.IsNullOrEmpty(pdc)) { - info.PrimaryDomainController = pdc; + primaryDomainController = pdc; } } } @@ -1842,7 +1862,12 @@ private static bool TryGetDomainInfoViaUncontrolledFallback(string domainName, L "DirectoryEntry tier: PDC lookup failed for {Domain}", domainName); } - return (true, info); + return (true, new DomainInfo( + name: name, + distinguishedName: distinguishedName, + forestName: forestName, + domainSid: domainSid, + primaryDomainController: primaryDomainController)); }).ConfigureAwait(false); } @@ -1979,12 +2004,13 @@ private static (bool Success, DomainInfo DomainInfo) ResolveDomainInfoFromConnec return (false, null); } - var info = new DomainInfo { - Name = Helpers.DistinguishedNameToDomain(defaultNc).ToUpper(), - DistinguishedName = defaultNc, - }; + var name = Helpers.DistinguishedNameToDomain(defaultNc).ToUpper(); + string domainSid = null; + string forestName = null; + string primaryDomainController = null; + IReadOnlyList domainControllers = null; if (!string.IsNullOrWhiteSpace(rootNc)) { - info.ForestName = Helpers.DistinguishedNameToDomain(rootNc).ToUpper(); + forestName = Helpers.DistinguishedNameToDomain(rootNc).ToUpper(); } // 2. Domain NC base search - objectSid + fsmoRoleOwner in one round-trip. @@ -1997,7 +2023,7 @@ private static (bool Success, DomainInfo DomainInfo) ResolveDomainInfoFromConnec if (domResp?.Entries != null && domResp.Entries.Count > 0) { var entry = new SearchResultEntryWrapper(domResp.Entries[0]); if (entry.TryGetSecurityIdentifier(out var sid) && !string.IsNullOrEmpty(sid)) { - info.DomainSid = sid.ToUpper(); + domainSid = sid.ToUpper(); } entry.TryGetProperty(LDAPProperties.FSMORoleOwner, out fsmoOwner); } @@ -2016,7 +2042,7 @@ private static (bool Success, DomainInfo DomainInfo) ResolveDomainInfoFromConnec if (pdcResp?.Entries != null && pdcResp.Entries.Count > 0) { var entry = new SearchResultEntryWrapper(pdcResp.Entries[0]); if (entry.TryGetProperty(LDAPProperties.DNSHostName, out var pdcName)) { - info.PrimaryDomainController = pdcName; + primaryDomainController = pdcName; } } } @@ -2041,16 +2067,22 @@ private static (bool Success, DomainInfo DomainInfo) ResolveDomainInfoFromConnec } } } - info.DomainControllers = dcs; - if (string.IsNullOrEmpty(info.PrimaryDomainController) && dcs.Count > 0) { - info.PrimaryDomainController = dcs[0]; + domainControllers = dcs; + if (string.IsNullOrEmpty(primaryDomainController) && dcs.Count > 0) { + primaryDomainController = dcs[0]; } } catch (Exception e) { log?.LogDebug(e, "Direct LDAP DC enumeration failed for {Domain}", domainName); } - return (true, info); + return (true, new DomainInfo( + name: name, + distinguishedName: defaultNc, + forestName: forestName, + domainSid: domainSid, + primaryDomainController: primaryDomainController, + domainControllers: domainControllers)); } public void ResetUtils() { diff --git a/test/unit/GPOLocalGroupProcessorTest.cs b/test/unit/GPOLocalGroupProcessorTest.cs index 8dedb0dd4..dfdb5ae5e 100644 --- a/test/unit/GPOLocalGroupProcessorTest.cs +++ b/test/unit/GPOLocalGroupProcessorTest.cs @@ -323,7 +323,7 @@ public async Task GPOLocalGroupProcessor_ReadGPOLocalGroups() { .Returns(mockComputerResults.ToAsyncEnumerable) .Returns(mockGCPFileSysPathResults.ToAsyncEnumerable) .Returns(Array.Empty>().ToAsyncEnumerable); - var domainInfo = new DomainInfo { Name = "TESTLAB.LOCAL" }; + var domainInfo = new DomainInfo(name: "TESTLAB.LOCAL"); mockLDAPUtils.Setup(x => x.GetDomainInfoAsync()).ReturnsAsync((true, domainInfo)); var processor = new GPOLocalGroupProcessor(mockLDAPUtils.Object); From 78ddc219c7858ea61ea3e59f5f749a94af3e86bd Mon Sep 17 00:00:00 2001 From: rvazarkar Date: Thu, 23 Apr 2026 19:14:30 -0400 Subject: [PATCH 03/20] chore: delete bad comments --- src/CommonLib/LdapUtils.cs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/CommonLib/LdapUtils.cs b/src/CommonLib/LdapUtils.cs index 0c4d2c345..684a17600 100644 --- a/src/CommonLib/LdapUtils.cs +++ b/src/CommonLib/LdapUtils.cs @@ -1453,7 +1453,7 @@ private static bool TryResolveHintViaUncontrolledGetDomain( /// /// All follow-up queries after the initial NC read are wrapped in try/catch and only /// populate optional fields - a failure here still returns a partially-filled - /// with = false. A failure + /// . A failure /// to acquire the connection or to read the default NC is fatal and returns (false, null). /// private static async Task<(bool Success, DomainInfo DomainInfo)> ResolveDomainInfoControlledAsyncCore( @@ -1635,8 +1635,7 @@ internal static bool TryStripNtdsSettingsPrefix(string fsmoRoleOwnerDn, out stri /// The call into Domain.GetDomain does not honor any flag /// beyond the username/password branches - server, port, SSL, signing, and cert-verification /// settings are all bypassed because that API performs its own DC discovery via the native - /// DS RPC stack. The resulting is marked with - /// = true so callers and tests can detect the path. + /// DS RPC stack. /// /// /// Every optional property access (Forest, PdcRoleOwner, DomainControllers, From 0b839f8e7d8c119aa3f87e415c1099396ab80d2b Mon Sep 17 00:00:00 2001 From: rvazarkar Date: Mon, 27 Apr 2026 12:11:49 -0400 Subject: [PATCH 04/20] chore: fix order of resolution in connection pool --- src/CommonLib/ConnectionPoolManager.cs | 24 +++++++++++++----------- 1 file changed, 13 insertions(+), 11 deletions(-) diff --git a/src/CommonLib/ConnectionPoolManager.cs b/src/CommonLib/ConnectionPoolManager.cs index a01bcf8dd..19d92909b 100644 --- a/src/CommonLib/ConnectionPoolManager.cs +++ b/src/CommonLib/ConnectionPoolManager.cs @@ -133,23 +133,14 @@ private string ResolveIdentifier(string identifier) { private (bool, string) GetDomainSidFromDomainName(string domainName) { if (Cache.GetDomainSidMapping(domainName, out var domainSid)) return (true, domainSid); - try { - var entry = Helpers.CreateDirectoryEntry($"LDAP://{domainName}", _ldapConfig); - if (entry.TryGetSecurityIdentifier(out var sid)) { - Cache.AddDomainSidMapping(domainName, sid); - return (true, sid); - } - } - catch { - //we expect this to fail sometimes - } - // Controlled replacement for LdapUtils.GetDomain + GetDirectoryEntry. We pass pool: null // because this method is called from inside GetPool -> ResolveIdentifier while resolving // the pool for this same domain; reusing the pool here would reenter GetLdapConnection and // recurse into GetDomainSidFromDomainName. With pool: null, GetDomainInfoStaticAsync falls // through to its direct-LDAP (one-shot LdapConnection) path, which still honors LdapConfig. // The call is sync-over-async to match the sibling pattern in GetLdapConnectionForServer. + // Tried before the legacy ADSI bind so that uncontrolled/serverless ADSI lookups do not + // run prior to ResolveIdentifier/GetPool/GetLdapConnectionForServer logic. var (infoOk, info) = LdapUtils .GetDomainInfoStaticAsync(domainName, _ldapConfig, _log) .GetAwaiter().GetResult(); @@ -158,6 +149,17 @@ private string ResolveIdentifier(string identifier) { return (true, info.DomainSid); } + try { + var entry = Helpers.CreateDirectoryEntry($"LDAP://{domainName}", _ldapConfig); + if (entry.TryGetSecurityIdentifier(out var sid)) { + Cache.AddDomainSidMapping(domainName, sid); + return (true, sid); + } + } + catch { + //we expect this to fail sometimes + } + foreach (var name in _translateNames) try { var account = new NTAccount(domainName, name); From 2e5c059a7a1a90341fc65bcca9fdc2ac1d8b7b6f Mon Sep 17 00:00:00 2001 From: rvazarkar Date: Tue, 28 Apr 2026 10:01:06 -0400 Subject: [PATCH 05/20] fix: add enrichment of sparse tier domain resolution via LDAP retry fix: ensure all paths respect the ldapconfig.server configuration --- src/CommonLib/LdapUtils.cs | 270 +++++++++++++++++++++++++++++--- test/unit/LDAPUtilsTest.cs | 311 +++++++++++++++++++++++++++++++++++++ 2 files changed, 562 insertions(+), 19 deletions(-) diff --git a/src/CommonLib/LdapUtils.cs b/src/CommonLib/LdapUtils.cs index 684a17600..0eb8aa8b9 100644 --- a/src/CommonLib/LdapUtils.cs +++ b/src/CommonLib/LdapUtils.cs @@ -509,6 +509,14 @@ public bool GetDomain(string domainName, out Domain domain) { domain = null; return false; } + + if (!string.IsNullOrWhiteSpace(_ldapConfig.Server)) { + _log.LogDebug( + "GetDomain(\"{Name}\", out Domain) short-circuited: Specific Server is set", + domainName); + domain = null; + return false; + } var cacheKey = domainName ?? _nullCacheKey; if (_domainCache.TryGetValue(cacheKey, out domain)) return true; @@ -540,13 +548,21 @@ public bool GetDomain(string domainName, out Domain domain) { } public static bool GetDomain(string domainName, LdapConfig ldapConfig, out Domain domain) { - if (ldapConfig == null || !ldapConfig.AllowFallbackToUncontrolledLdap) { + if (ldapConfig is not { AllowFallbackToUncontrolledLdap: true }) { Logging.Logger.LogDebug( "Static GetDomain(\"{DomainName}\") short-circuited: AllowFallbackToUncontrolledLdap is disabled", domainName); domain = null; return false; } + + if (!string.IsNullOrWhiteSpace(ldapConfig.Server)) { + Logging.Logger.LogDebug( + "GetDomain(\"{Name}\", out Domain) short-circuited: Specific Server is set", + domainName); + domain = null; + return false; + } if (_domainCache.TryGetValue(domainName, out domain)) return true; @@ -591,6 +607,13 @@ public bool GetDomain(out Domain domain) { domain = null; return false; } + + if (!string.IsNullOrWhiteSpace(_ldapConfig.Server)) { + _log.LogDebug( + "GetDomain() short-circuited: Specific Server is set"); + domain = null; + return false; + } if (_domainCache.TryGetValue(_nullCacheKey, out domain)) return true; @@ -1149,6 +1172,79 @@ public void SetLdapConfig(LdapConfig config) { return (false, default); } + /// + /// Counts the populated fields on a as a coarse measure of how + /// much information a particular resolution tier produced. Used by + /// to ensure a richer record published by a later tier replaces a sparser record published + /// by an earlier tier, instead of being dropped by ConcurrentDictionary.TryAdd's + /// first-writer-wins semantics. is treated as + /// "populated" only when non-empty because the constructor coalesces null to an empty array. + /// + internal static int CompletenessScore(DomainInfo info) { + if (info == null) return -1; + var score = 0; + if (!string.IsNullOrEmpty(info.Name)) score++; + if (!string.IsNullOrEmpty(info.DistinguishedName)) score++; + if (!string.IsNullOrEmpty(info.ForestName)) score++; + if (!string.IsNullOrEmpty(info.DomainSid)) score++; + if (!string.IsNullOrEmpty(info.NetBiosName)) score++; + if (!string.IsNullOrEmpty(info.PrimaryDomainController)) score++; + if (info.DomainControllers != null && info.DomainControllers.Count > 0) score++; + return score; + } + + /// + /// Inserts at in + /// , replacing any existing entry only when the candidate has + /// strictly more populated fields (per ). Equal scores + /// preserve the existing entry to keep cache writes idempotent under concurrent resolution. + /// + /// + /// The four resolution tiers behind and + /// populate different attribute subsets - notably + /// only the pool-driven path queries + /// CN=Partitions for . Because the static helper + /// is invoked re-entrantly from + /// during pool acquisition, the sparser one-shot direct-LDAP record reaches the cache first; + /// without this helper the subsequent pool-derived record would be silently discarded by + /// TryAdd and every later cache hit would observe the partial record. + /// + internal static void CacheDomainInfo(string key, DomainInfo candidate) { + if (key == null || candidate == null) return; + _domainInfoCache.AddOrUpdate( + key, + candidate, + (_, existing) => CompletenessScore(candidate) > CompletenessScore(existing) + ? candidate + : existing); + } + + /// + /// Picks the richer of and , guarding + /// against cross-domain leakage when an enrichment retry binds to a DC discovered by an + /// earlier tier. Returns unchanged when + /// is null, when the two records describe different canonical domain names, or when the + /// enriched record does not have strictly more populated fields per + /// . + /// + /// + /// The name-equality guard is deliberately strict: an enrichment retry that binds to the + /// PDC discovered by ADSI (or any other tier) and reads a different defaultNamingContext + /// than the seed indicates the retry landed on a DC that is not actually in the requested + /// domain (cross-forest leak, decommissioned host, mismatched config.Server). In that + /// case the seed is preferred even though the retry produced a higher score, because caching + /// the retry's record under the seed's cache key would silently associate the wrong SID and + /// NetBIOS name with that domain. + /// + internal static DomainInfo SelectRicherDomainInfo(DomainInfo seed, DomainInfo enriched) { + if (seed == null) return enriched; + if (enriched == null) return seed; + if (!string.Equals(seed.Name, enriched.Name, StringComparison.OrdinalIgnoreCase)) { + return seed; + } + return CompletenessScore(enriched) > CompletenessScore(seed) ? enriched : seed; + } + /// /// Resolves a for the specified domain, preferring a controlled /// LDAP path that honors the configured (server, port, SSL, @@ -1177,7 +1273,7 @@ public void SetLdapConfig(LdapConfig config) { // therefore honors every flag on LdapConfig. var (controlledOk, controlledInfo) = await ResolveDomainInfoControlledAsync(domainName); if (controlledOk) { - _domainInfoCache.TryAdd(cacheKey, controlledInfo); + CacheDomainInfo(cacheKey, controlledInfo); return (true, controlledInfo); } @@ -1194,7 +1290,7 @@ public void SetLdapConfig(LdapConfig config) { var (directOk, directInfo) = await TryResolveDomainInfoViaDirectLdapAsync(effectiveDomain, _ldapConfig, _log); if (directOk) { - _domainInfoCache.TryAdd(cacheKey, directInfo); + CacheDomainInfo(cacheKey, directInfo); return (true, directInfo); } } @@ -1206,7 +1302,13 @@ public void SetLdapConfig(LdapConfig config) { var (adsiOk, adsiInfo) = await TryResolveDomainInfoViaDirectoryEntryAsync(effectiveDomain, _ldapConfig, _log); if (adsiOk) { - _domainInfoCache.TryAdd(cacheKey, adsiInfo); + // ADSI populates PrimaryDomainController via DsGetDcName. Retrying the + // direct-LDAP path against that discovered DC closes the score gap when the + // original raw-LDAP attempt failed only because the domain name itself had + // no resolvable A/SRV record from this host. + adsiInfo = await TryEnrichDomainInfoViaDirectLdapAsync( + effectiveDomain, adsiInfo, _ldapConfig, _log); + CacheDomainInfo(cacheKey, adsiInfo); return (true, adsiInfo); } } @@ -1214,7 +1316,13 @@ public void SetLdapConfig(LdapConfig config) { // Opt-in fallback. TryGetDomainInfoViaUncontrolledFallback short-circuits when // AllowFallbackToUncontrolledLdap is disabled, so when the flag is off this call is a no-op. if (TryGetDomainInfoViaUncontrolledFallback(effectiveDomain, _ldapConfig, _log, out var fallbackInfo)) { - _domainInfoCache.TryAdd(cacheKey, fallbackInfo); + // Same enrichment opportunity as the ADSI tier - the uncontrolled fallback + // populates both PrimaryDomainController and DomainControllers but never + // NetBiosName, so the discovered DC name is enough to reach a score-7 record + // via a controlled retry. + fallbackInfo = await TryEnrichDomainInfoViaDirectLdapAsync( + effectiveDomain, fallbackInfo, _ldapConfig, _log); + CacheDomainInfo(cacheKey, fallbackInfo); return (true, fallbackInfo); } @@ -1260,7 +1368,7 @@ public void SetLdapConfig(LdapConfig config) { // that can express fine-grained LdapConfig.AuthType and LdapConfig.DisableCertVerification. var (directOk, directInfo) = await TryResolveDomainInfoViaDirectLdapAsync(domainName, config, log); if (directOk) { - _domainInfoCache.TryAdd(domainName, directInfo); + CacheDomainInfo(domainName, directInfo); return (true, directInfo); } @@ -1269,12 +1377,19 @@ public void SetLdapConfig(LdapConfig config) { // the domain name itself, at the cost of not honoring every LdapConfig flag. var (adsiOk, adsiInfo) = await TryResolveDomainInfoViaDirectoryEntryAsync(domainName, config, log); if (adsiOk) { - _domainInfoCache.TryAdd(domainName, adsiInfo); + // Retry the direct-LDAP path against the DC discovered by ADSI so the cached + // record reaches the same shape as the pool/one-shot tiers. Mirrors the matching + // enrichment in the instance overload. + adsiInfo = await TryEnrichDomainInfoViaDirectLdapAsync( + domainName, adsiInfo, config, log); + CacheDomainInfo(domainName, adsiInfo); return (true, adsiInfo); } if (TryGetDomainInfoViaUncontrolledFallback(domainName, config, log, out var fallbackInfo)) { - _domainInfoCache.TryAdd(domainName, fallbackInfo); + fallbackInfo = await TryEnrichDomainInfoViaDirectLdapAsync( + domainName, fallbackInfo, config, log); + CacheDomainInfo(domainName, fallbackInfo); return (true, fallbackInfo); } @@ -1397,7 +1512,14 @@ private static bool TryResolveHintViaUncontrolledGetDomain( domainName = null; // Hard gate: uncontrolled calls require explicit opt-in. - if (config == null || !config.AllowFallbackToUncontrolledLdap) return false; + if (config is not { AllowFallbackToUncontrolledLdap: true }) return false; + + if (!string.IsNullOrWhiteSpace(config.Server)) { + log.LogDebug( + "TryResolveHintViaUncontrolledGetDomain(\"{Name}\", out string DomainName) short-circuited: Specific Server is set", + domainName); + return false; + } lock (_uncontrolledGetDomainHintLock) { // Previously resolved or previously failed - either way, no new RPC. @@ -1635,7 +1757,7 @@ internal static bool TryStripNtdsSettingsPrefix(string fsmoRoleOwnerDn, out stri /// The call into Domain.GetDomain does not honor any flag /// beyond the username/password branches - server, port, SSL, signing, and cert-verification /// settings are all bypassed because that API performs its own DC discovery via the native - /// DS RPC stack. + /// DS RPC stack. /// /// /// Every optional property access (Forest, PdcRoleOwner, DomainControllers, @@ -1649,9 +1771,17 @@ private static bool TryGetDomainInfoViaUncontrolledFallback(string domainName, L info = null; // Hard gate - when the flag is off this method is a no-op regardless of what the // surrounding LDAP config looks like. - if (config == null || !config.AllowFallbackToUncontrolledLdap) { + if (config is not { AllowFallbackToUncontrolledLdap: true }) { return false; } + + if (!string.IsNullOrWhiteSpace(config.Server)) { + log.LogDebug( + "TryGetDomainInfoViaUncontrolledFallback(\"{Name}\", out DomainInfo info) short-circuited: Specific Server is set", + domainName); + return false; + } + try { // Matches the DirectoryContext construction in the legacy GetDomain overloads so @@ -1927,13 +2057,27 @@ private static LdapConnection TryBindOneShotLdapConnection( return null; } + /// + /// Selects the bind target for the static one-shot LDAP path: + /// when explicitly set, otherwise . Mirrors the equivalent + /// short-circuit in LdapConnectionPool.CreateNewConnection so both controlled tiers + /// honor the user's override consistently. + /// + internal static string ResolveOneShotBindTarget(string domainName, LdapConfig config) { + if (config != null && !string.IsNullOrWhiteSpace(config.Server)) { + return config.Server; + } + return domainName; + } + /// /// Controlled resolver used when no is available to /// route queries through. Binds a one-shot - /// directly to (treating it as a DNS domain name that resolves - /// to a DC via standard round-robin A/SRV records) and populates a - /// by issuing the same rootDSE + Base + Subtree queries as - /// , just over a private connection. + /// to when set, otherwise to + /// (treating it as a DNS domain name that resolves to a DC via standard round-robin A/SRV + /// records) and populates a by issuing the same rootDSE + Base + + /// Subtree queries as , just over a + /// private connection. /// /// /// Intended as the intermediate step in between the @@ -1949,7 +2093,12 @@ private static LdapConnection TryBindOneShotLdapConnection( return (false, null); } - var connection = TryBindOneShotLdapConnection(domainName, config, log); + // Honor LdapConfig.Server the same way the pool does. The resolved DomainInfo's Name is + // still derived from the bound DC's defaultNamingContext, so a Server pointed at a DC + // in a different domain than domainName will yield a record whose Name does not match + // domainName - same behavior as the pool, and the user's explicit override of intent. + var target = ResolveOneShotBindTarget(domainName, config); + var connection = TryBindOneShotLdapConnection(target, config, log); if (connection == null) { return (false, null); } @@ -1966,13 +2115,70 @@ private static LdapConnection TryBindOneShotLdapConnection( }).ConfigureAwait(false); } + /// + /// Re-runs the direct-LDAP resolution against a DC name discovered by a sparser tier + /// (ADSI or the uncontrolled Domain.GetDomain fallback) so the cached record can + /// reach parity with the pool/one-shot tiers when the original direct-LDAP attempt + /// failed because the domain name had no usable DNS A/SRV record from the calling host. + /// + /// + /// Skips the bind entirely when the seed already has a maximum + /// , when is set (the + /// server-pinning contract forbids binding to any host other than the configured one, + /// and consumers are warned that data loss is the intended trade-off), when no bind + /// target can be derived from the seed ( + /// falling back to the first entry of ), or + /// when the bind itself fails. In any of these cases the original seed is returned + /// unchanged. When the retry succeeds, the merged result is selected by + /// , which enforces the canonical-name guard so a + /// retry that lands on a DC outside the requested domain does not poison the cache. + /// : + internal static async Task TryEnrichDomainInfoViaDirectLdapAsync( + string domainName, DomainInfo seed, LdapConfig config, ILogger log) { + if (seed == null) return null; + if (CompletenessScore(seed) >= 7 || string.IsNullOrWhiteSpace(domainName) || config == null) return seed; + + string target; + if (!string.IsNullOrWhiteSpace(config.Server)) { + target = config.Server; + } else if (!string.IsNullOrWhiteSpace(seed.PrimaryDomainController)) { + target = seed.PrimaryDomainController; + } else if (seed.DomainControllers is { Count: > 0 }) { + target = seed.DomainControllers[0]; + } else { + target = null; + } + + if (string.IsNullOrWhiteSpace(target)) return seed; + + var connection = TryBindOneShotLdapConnection(target, config, log); + if (connection == null) { + log?.LogDebug( + "Direct LDAP enrichment bind failed for {Domain} via discovered DC {Target}", + domainName, target); + return seed; + } + + return await Task.Run(() => { + try { + var (ok, enriched) = ResolveDomainInfoFromConnection(connection, domainName, log); + if (!ok) return seed; + return SelectRicherDomainInfo(seed, enriched); + } + finally { + connection.Dispose(); + } + }).ConfigureAwait(false); + } + /// /// Synchronous body of . Uses /// SendRequest directly rather than the pool's Query because no pool is /// available on this path. Follows the same attribute shape as /// : rootDSE → domain NC base → - /// PDC server DN → DC subtree enumeration, each wrapped independently so a partial - /// failure still returns a usable . + /// PDC server DN → CN=Partitions crossRef (NetBIOS) → DC subtree enumeration, each + /// wrapped independently so a partial failure still returns a usable + /// . /// private static (bool Success, DomainInfo DomainInfo) ResolveDomainInfoFromConnection( LdapConnection connection, string domainName, ILogger log) { @@ -2007,6 +2213,7 @@ private static (bool Success, DomainInfo DomainInfo) ResolveDomainInfoFromConnec string domainSid = null; string forestName = null; string primaryDomainController = null; + string netBiosName = null; IReadOnlyList domainControllers = null; if (!string.IsNullOrWhiteSpace(rootNc)) { forestName = Helpers.DistinguishedNameToDomain(rootNc).ToUpper(); @@ -2050,7 +2257,31 @@ private static (bool Success, DomainInfo DomainInfo) ResolveDomainInfoFromConnec } } - // 4. Domain controller enumeration - same filter as CommonFilters.DomainControllers. + // 4. NetBIOS name via the crossRef object under CN=Partitions whose nCName matches + // the domain NC. Mirrors the partitions lookup in ResolveDomainInfoControlledAsyncCore + // so the static one-shot tier produces the same field shape as the pool tier. + if (!string.IsNullOrWhiteSpace(configNc)) { + try { + var nbReq = new SearchRequest( + $"CN=Partitions,{configNc}", + $"(&(objectClass=crossRef)({LDAPProperties.NCName}={defaultNc}))", + SearchScope.OneLevel, + new[] { LDAPProperties.NetbiosName }); + var nbResp = (SearchResponse)connection.SendRequest(nbReq); + if (nbResp?.Entries is { Count: > 0 }) { + var entry = new SearchResultEntryWrapper(nbResp.Entries[0]); + if (entry.TryGetProperty(LDAPProperties.NetbiosName, out var nb) && + !string.IsNullOrEmpty(nb)) { + netBiosName = nb; + } + } + } + catch (Exception e) { + log?.LogDebug(e, "Direct LDAP netbios lookup failed for {Domain}", domainName); + } + } + + // 5. Domain controller enumeration - same filter as CommonFilters.DomainControllers. try { var dcs = new List(); var dcReq = new SearchRequest( @@ -2080,6 +2311,7 @@ private static (bool Success, DomainInfo DomainInfo) ResolveDomainInfoFromConnec distinguishedName: defaultNc, forestName: forestName, domainSid: domainSid, + netBiosName: netBiosName, primaryDomainController: primaryDomainController, domainControllers: domainControllers)); } diff --git a/test/unit/LDAPUtilsTest.cs b/test/unit/LDAPUtilsTest.cs index 6f9966f15..a14f411d7 100644 --- a/test/unit/LDAPUtilsTest.cs +++ b/test/unit/LDAPUtilsTest.cs @@ -795,5 +795,316 @@ public void ResolveEffectiveDomainHint_WhitespaceCurrentUserDomain_FallsThroughT Assert.NotEqual(" ", result); } + // --------------------------------------------------------------------------- + // CompletenessScore / CacheDomainInfo (H-1 regression coverage) + // --------------------------------------------------------------------------- + + [Fact] + public void CompletenessScore_NullInfo_ReturnsNegativeOne() { + Assert.Equal(-1, LdapUtils.CompletenessScore(null)); + } + + [Fact] + public void CompletenessScore_EmptyInfo_ReturnsZero() { + Assert.Equal(0, LdapUtils.CompletenessScore(new DomainInfo())); + } + + [Fact] + public void CompletenessScore_AllFieldsPopulated_ReturnsSeven() { + var info = new DomainInfo( + name: "CONTOSO.LOCAL", + distinguishedName: "DC=contoso,DC=local", + forestName: "CONTOSO.LOCAL", + domainSid: "S-1-5-21-1-2-3", + netBiosName: "CONTOSO", + primaryDomainController: "dc01.contoso.local", + domainControllers: new[] { "dc01.contoso.local" }); + Assert.Equal(7, LdapUtils.CompletenessScore(info)); + } + + [Fact] + public void CompletenessScore_EmptyDomainControllersList_ContributesZero() { + var info = new DomainInfo(name: "CONTOSO.LOCAL", domainControllers: Array.Empty()); + Assert.Equal(1, LdapUtils.CompletenessScore(info)); + } + + [Fact] + public void CompletenessScore_EmptyStringFields_ContributeZero() { + var info = new DomainInfo(name: "", distinguishedName: "", forestName: ""); + Assert.Equal(0, LdapUtils.CompletenessScore(info)); + } + + [Fact] + public async Task CacheDomainInfo_RicherRecordReplacesSparserRecord() { + new LdapUtils().ResetUtils(); + const string key = "completeness-upgrade.test"; + + // Sparse record like the one-shot direct-LDAP tier produces (no NetBiosName). + var sparse = new DomainInfo( + name: "COMPLETENESS-UPGRADE.TEST", + distinguishedName: "DC=completeness-upgrade,DC=test", + domainSid: "S-1-5-21-1-2-3"); + LdapUtils.CacheDomainInfo(key, sparse); + + // Rich record like the pool-driven controlled tier produces. + var rich = new DomainInfo( + name: "COMPLETENESS-UPGRADE.TEST", + distinguishedName: "DC=completeness-upgrade,DC=test", + domainSid: "S-1-5-21-1-2-3", + netBiosName: "COMPLETE", + primaryDomainController: "dc01.completeness-upgrade.test", + domainControllers: new[] { "dc01.completeness-upgrade.test" }); + LdapUtils.CacheDomainInfo(key, rich); + + var (ok, cached) = await LdapUtils.GetDomainInfoStaticAsync(key, new LdapConfig { + Server = "unreachable.invalid.test", + AllowFallbackToUncontrolledLdap = false + }); + Assert.True(ok); + Assert.Equal("COMPLETE", cached.NetBiosName); + Assert.Equal("dc01.completeness-upgrade.test", cached.PrimaryDomainController); + } + + [Fact] + public async Task CacheDomainInfo_SparserRecordDoesNotReplaceRicher() { + new LdapUtils().ResetUtils(); + const string key = "completeness-no-downgrade.test"; + + var rich = new DomainInfo( + name: "COMPLETENESS-NO-DOWNGRADE.TEST", + distinguishedName: "DC=completeness-no-downgrade,DC=test", + domainSid: "S-1-5-21-9-9-9", + netBiosName: "RICHFIRST", + primaryDomainController: "dc01.completeness-no-downgrade.test", + domainControllers: new[] { "dc01.completeness-no-downgrade.test" }); + LdapUtils.CacheDomainInfo(key, rich); + + var sparse = new DomainInfo( + name: "COMPLETENESS-NO-DOWNGRADE.TEST", + distinguishedName: "DC=completeness-no-downgrade,DC=test", + domainSid: "S-1-5-21-9-9-9"); + LdapUtils.CacheDomainInfo(key, sparse); + + var (ok, cached) = await LdapUtils.GetDomainInfoStaticAsync(key, new LdapConfig { + Server = "unreachable.invalid.test", + AllowFallbackToUncontrolledLdap = false + }); + Assert.True(ok); + Assert.Equal("RICHFIRST", cached.NetBiosName); + } + + [Fact] + public async Task CacheDomainInfo_EqualScoreDoesNotReplaceExisting() { + new LdapUtils().ResetUtils(); + const string key = "completeness-equal.test"; + + var first = new DomainInfo(name: "COMPLETENESS-EQUAL.TEST", domainSid: "S-1-5-21-1-1-1"); + LdapUtils.CacheDomainInfo(key, first); + + var second = new DomainInfo(name: "COMPLETENESS-EQUAL.TEST", domainSid: "S-1-5-21-2-2-2"); + LdapUtils.CacheDomainInfo(key, second); + + var (ok, cached) = await LdapUtils.GetDomainInfoStaticAsync(key, new LdapConfig { + Server = "unreachable.invalid.test", + AllowFallbackToUncontrolledLdap = false + }); + Assert.True(ok); + Assert.Equal("S-1-5-21-1-1-1", cached.DomainSid); + } + + [Fact] + public async Task CacheDomainInfo_NullKeyOrCandidate_NoOp() { + new LdapUtils().ResetUtils(); + const string key = "completeness-noop.test"; + + LdapUtils.CacheDomainInfo(null, new DomainInfo(name: "IGNORED")); + LdapUtils.CacheDomainInfo(key, null); + + // Static helper should fail through every tier because nothing was cached and the + // server is unreachable with fallback disabled. + var (ok, cached) = await LdapUtils.GetDomainInfoStaticAsync(key, new LdapConfig { + Server = "unreachable.invalid.test", + AllowFallbackToUncontrolledLdap = false + }); + Assert.False(ok); + Assert.Null(cached); + } + + // --------------------------------------------------------------------------- + // SelectRicherDomainInfo / TryEnrichDomainInfoViaDirectLdapAsync coverage + // --------------------------------------------------------------------------- + + [Fact] + public void SelectRicherDomainInfo_NullEnriched_ReturnsSeed() { + var seed = new DomainInfo(name: "CONTOSO.LOCAL", domainSid: "S-1-5-21-1-2-3"); + Assert.Same(seed, LdapUtils.SelectRicherDomainInfo(seed, null)); + } + + [Fact] + public void SelectRicherDomainInfo_NullSeed_ReturnsEnriched() { + var enriched = new DomainInfo(name: "CONTOSO.LOCAL"); + Assert.Same(enriched, LdapUtils.SelectRicherDomainInfo(null, enriched)); + } + + [Fact] + public void SelectRicherDomainInfo_NameMismatch_ReturnsSeed() { + var seed = new DomainInfo(name: "CONTOSO.LOCAL", domainSid: "S-1-5-21-1-2-3"); + // Enriched is technically richer but describes a different domain - the guard must + // reject it to prevent caching the wrong SID/NetBIOS under the seed's cache key. + var enriched = new DomainInfo( + name: "FABRIKAM.LOCAL", + distinguishedName: "DC=fabrikam,DC=local", + forestName: "FABRIKAM.LOCAL", + domainSid: "S-1-5-21-9-9-9", + netBiosName: "FABRIKAM", + primaryDomainController: "dc01.fabrikam.local", + domainControllers: new[] { "dc01.fabrikam.local" }); + Assert.Same(seed, LdapUtils.SelectRicherDomainInfo(seed, enriched)); + } + + [Fact] + public void SelectRicherDomainInfo_NameDiffersOnlyInCase_AcceptsEnriched() { + var seed = new DomainInfo(name: "CONTOSO.LOCAL", domainSid: "S-1-5-21-1-2-3"); + var enriched = new DomainInfo( + name: "contoso.local", + distinguishedName: "DC=contoso,DC=local", + domainSid: "S-1-5-21-1-2-3", + netBiosName: "CONTOSO"); + Assert.Same(enriched, LdapUtils.SelectRicherDomainInfo(seed, enriched)); + } + + [Fact] + public void SelectRicherDomainInfo_LowerScore_ReturnsSeed() { + var seed = new DomainInfo( + name: "CONTOSO.LOCAL", + distinguishedName: "DC=contoso,DC=local", + domainSid: "S-1-5-21-1-2-3"); + var enriched = new DomainInfo(name: "CONTOSO.LOCAL"); + Assert.Same(seed, LdapUtils.SelectRicherDomainInfo(seed, enriched)); + } + + [Fact] + public void SelectRicherDomainInfo_EqualScore_ReturnsSeed() { + var seed = new DomainInfo(name: "CONTOSO.LOCAL", domainSid: "S-1-5-21-1-2-3"); + var enriched = new DomainInfo(name: "CONTOSO.LOCAL", domainSid: "S-1-5-21-9-9-9"); + Assert.Same(seed, LdapUtils.SelectRicherDomainInfo(seed, enriched)); + } + + [Fact] + public void SelectRicherDomainInfo_HigherScore_ReturnsEnriched() { + var seed = new DomainInfo( + name: "CONTOSO.LOCAL", + distinguishedName: "DC=contoso,DC=local", + domainSid: "S-1-5-21-1-2-3", + primaryDomainController: "dc01.contoso.local"); + var enriched = new DomainInfo( + name: "CONTOSO.LOCAL", + distinguishedName: "DC=contoso,DC=local", + forestName: "CONTOSO.LOCAL", + domainSid: "S-1-5-21-1-2-3", + netBiosName: "CONTOSO", + primaryDomainController: "dc01.contoso.local", + domainControllers: new[] { "dc01.contoso.local" }); + Assert.Same(enriched, LdapUtils.SelectRicherDomainInfo(seed, enriched)); + } + + [Fact] + public async Task TryEnrichDomainInfoViaDirectLdapAsync_NullSeed_ReturnsNull() { + var result = await LdapUtils.TryEnrichDomainInfoViaDirectLdapAsync( + "CONTOSO.LOCAL", null, new LdapConfig(), log: null); + Assert.Null(result); + } + + [Fact] + public async Task TryEnrichDomainInfoViaDirectLdapAsync_FullScoreSeed_ReturnsSeedWithoutBinding() { + // A score-7 seed with PDC pointing at an unreachable host - if the helper attempted + // a bind it would either time out or fail; returning the seed identity proves the + // fast-path skipped the bind entirely. + var seed = new DomainInfo( + name: "CONTOSO.LOCAL", + distinguishedName: "DC=contoso,DC=local", + forestName: "CONTOSO.LOCAL", + domainSid: "S-1-5-21-1-2-3", + netBiosName: "CONTOSO", + primaryDomainController: "unreachable.invalid.test", + domainControllers: new[] { "unreachable.invalid.test" }); + var result = await LdapUtils.TryEnrichDomainInfoViaDirectLdapAsync( + "CONTOSO.LOCAL", seed, new LdapConfig(), log: null); + Assert.Same(seed, result); + } + + [Fact] + public async Task TryEnrichDomainInfoViaDirectLdapAsync_NoBindTarget_ReturnsSeed() { + // No PrimaryDomainController, no DomainControllers - nothing to bind to, helper must + // return the seed without attempting any network I/O. + var seed = new DomainInfo( + name: "CONTOSO.LOCAL", + distinguishedName: "DC=contoso,DC=local", + domainSid: "S-1-5-21-1-2-3"); + var result = await LdapUtils.TryEnrichDomainInfoViaDirectLdapAsync( + "CONTOSO.LOCAL", seed, new LdapConfig(), log: null); + Assert.Same(seed, result); + } + + [Fact] + public async Task TryEnrichDomainInfoViaDirectLdapAsync_NullConfig_ReturnsSeed() { + var seed = new DomainInfo( + name: "CONTOSO.LOCAL", + primaryDomainController: "dc01.contoso.local"); + var result = await LdapUtils.TryEnrichDomainInfoViaDirectLdapAsync( + "CONTOSO.LOCAL", seed, config: null, log: null); + Assert.Same(seed, result); + } + + [Fact] + public async Task TryEnrichDomainInfoViaDirectLdapAsync_ServerPinned_BindFailureReturnsSeed() { + // LdapConfig.Server pins all LDAP traffic to the configured host. Enrichment honors the + // pin by binding to config.Server rather than the seed's discovered PDC. When the pinned + // host is unreachable the bind fails and the seed is returned unchanged - this covers + // both the pin-respect and the bind-failure-fallback paths in one assertion. + var seed = new DomainInfo( + name: "CONTOSO.LOCAL", + distinguishedName: "DC=contoso,DC=local", + domainSid: "S-1-5-21-1-2-3", + primaryDomainController: "seed-pdc.invalid.test"); + var config = new LdapConfig { Server = "pinned-server.invalid.test" }; + var result = await LdapUtils.TryEnrichDomainInfoViaDirectLdapAsync( + "CONTOSO.LOCAL", seed, config, log: null); + Assert.Same(seed, result); + } + + // --------------------------------------------------------------------------- + // ResolveOneShotBindTarget + // --------------------------------------------------------------------------- + + [Fact] + public void ResolveOneShotBindTarget_ServerSet_OverridesDomainName() { + var config = new LdapConfig { Server = "dc01.contoso.local" }; + Assert.Equal("dc01.contoso.local", + LdapUtils.ResolveOneShotBindTarget("CONTOSO.LOCAL", config)); + } + + [Fact] + public void ResolveOneShotBindTarget_ServerNull_FallsBackToDomainName() { + var config = new LdapConfig { Server = null }; + Assert.Equal("CONTOSO.LOCAL", + LdapUtils.ResolveOneShotBindTarget("CONTOSO.LOCAL", config)); + } + + [Theory] + [InlineData("")] + [InlineData(" ")] + public void ResolveOneShotBindTarget_ServerWhitespace_FallsBackToDomainName(string server) { + var config = new LdapConfig { Server = server }; + Assert.Equal("CONTOSO.LOCAL", + LdapUtils.ResolveOneShotBindTarget("CONTOSO.LOCAL", config)); + } + + [Fact] + public void ResolveOneShotBindTarget_NullConfig_ReturnsDomainName() { + Assert.Equal("CONTOSO.LOCAL", + LdapUtils.ResolveOneShotBindTarget("CONTOSO.LOCAL", config: null)); + } + } } \ No newline at end of file From d3e697a25c20a7fe43749013e0dc2af44843d7a7 Mon Sep 17 00:00:00 2001 From: rvazarkar Date: Tue, 28 Apr 2026 11:59:16 -0400 Subject: [PATCH 06/20] fix: several casing violations in various dictionaries that would lead to cache misses fix: domain/sid mapping properly caches in both directions --- src/CommonLib/Cache.cs | 29 +++-- src/CommonLib/LdapConnectionPool.cs | 2 +- src/CommonLib/LdapUtils.cs | 11 +- .../Processors/GPOLocalGroupProcessor.cs | 7 +- test/unit/CacheTest.cs | 102 ++++++++++++++++++ 5 files changed, 133 insertions(+), 18 deletions(-) diff --git a/src/CommonLib/Cache.cs b/src/CommonLib/Cache.cs index e3ba46f23..5bd1379df 100644 --- a/src/CommonLib/Cache.cs +++ b/src/CommonLib/Cache.cs @@ -26,9 +26,9 @@ private Cache() { ValueToIdCache = new ConcurrentDictionary(); IdToTypeCache = new ConcurrentDictionary(); - GlobalCatalogCache = new ConcurrentDictionary(); + GlobalCatalogCache = new ConcurrentDictionary(StringComparer.OrdinalIgnoreCase); MachineSidCache = new ConcurrentDictionary(); - SIDToDomainCache = new ConcurrentDictionary(); + SIDToDomainCache = new ConcurrentDictionary(StringComparer.OrdinalIgnoreCase); } [DataMember] public ConcurrentDictionary GlobalCatalogCache { get; private set; } @@ -46,13 +46,22 @@ private Cache() [IgnoreDataMember] private static Cache CacheInstance { get; set; } /// - /// Add a SID to/from Domain mapping to the cache + /// Add a SID/Domain-name pair to the cache. Writes both directions (SID→Name and + /// Name→SID) so a successful resolution by any tier benefits subsequent lookups in + /// either direction. Existing entries are preserved (TryAdd semantics) — first + /// resolver wins. /// - /// - /// + /// A SID or a domain name. + /// The corresponding domain name or SID. internal static void AddDomainSidMapping(string key, string value) { - CacheInstance?.SIDToDomainCache.TryAdd(key, value); + if (CacheInstance == null) return; + if (string.IsNullOrEmpty(key) || string.IsNullOrEmpty(value)) return; + CacheInstance.SIDToDomainCache.TryAdd(key, value); + // Bidirectional: SIDs (S-…) and DNS domain names cannot collide as keys, so writing + // the reverse mapping makes a successful resolution by any tier visible to lookups + // in either direction. + CacheInstance.SIDToDomainCache.TryAdd(value, key); } /// @@ -102,7 +111,7 @@ internal static void AddGCCache(string key, string[] value) internal static bool GetGCCache(string key, out string[] value) { - if (CacheInstance != null) return CacheInstance.GlobalCatalogCache.TryGetValue(key.ToUpper(), out value); + if (CacheInstance != null) return CacheInstance.GlobalCatalogCache.TryGetValue(key, out value); value = null; return false; } @@ -184,9 +193,11 @@ private static void CreateMissingDictionaries() { CacheInstance ??= new Cache(); CacheInstance.IdToTypeCache ??= new ConcurrentDictionary(); - CacheInstance.GlobalCatalogCache ??= new ConcurrentDictionary(); + CacheInstance.GlobalCatalogCache ??= + new ConcurrentDictionary(StringComparer.OrdinalIgnoreCase); CacheInstance.MachineSidCache ??= new ConcurrentDictionary(); - CacheInstance.SIDToDomainCache ??= new ConcurrentDictionary(); + CacheInstance.SIDToDomainCache ??= + new ConcurrentDictionary(StringComparer.OrdinalIgnoreCase); CacheInstance.ValueToIdCache ??= new ConcurrentDictionary(); } } diff --git a/src/CommonLib/LdapConnectionPool.cs b/src/CommonLib/LdapConnectionPool.cs index fff843d9c..6b9337092 100644 --- a/src/CommonLib/LdapConnectionPool.cs +++ b/src/CommonLib/LdapConnectionPool.cs @@ -42,7 +42,7 @@ internal class LdapConnectionPool : IDisposable { private readonly IMetricRouter _metric; // Tracks domains we know we've determined we shouldn't try to connect to - private static readonly ConcurrentHashSet ExcludedDomains = new(); + private static readonly ConcurrentHashSet ExcludedDomains = new(StringComparer.OrdinalIgnoreCase); public LdapConnectionPool(string identifier, string poolIdentifier, LdapConfig config, IPortScanner scanner = null, NativeMethods nativeMethods = null, ILogger log = null, IMetricRouter metric = null) { diff --git a/src/CommonLib/LdapUtils.cs b/src/CommonLib/LdapUtils.cs index 0eb8aa8b9..9d32d365a 100644 --- a/src/CommonLib/LdapUtils.cs +++ b/src/CommonLib/LdapUtils.cs @@ -31,7 +31,8 @@ namespace SharpHoundCommonLib { public class LdapUtils : ILdapUtils { //This cache is indexed by domain sid - private static ConcurrentDictionary _domainCache = new(); + private static ConcurrentDictionary _domainCache = + new(StringComparer.OrdinalIgnoreCase); private static ConcurrentDictionary _domainInfoCache = new(StringComparer.OrdinalIgnoreCase); private static ConcurrentHashSet _domainControllers = new(StringComparer.OrdinalIgnoreCase); @@ -2317,10 +2318,10 @@ private static (bool Success, DomainInfo DomainInfo) ResolveDomainInfoFromConnec } public void ResetUtils() { - _unresolvablePrincipals = new ConcurrentHashSet(StringComparer.OrdinalIgnoreCase); - _domainCache = new ConcurrentDictionary(); - _domainInfoCache = new ConcurrentDictionary(StringComparer.OrdinalIgnoreCase); - _domainControllers = new ConcurrentHashSet(StringComparer.OrdinalIgnoreCase); + _unresolvablePrincipals.Clear(); + _domainCache.Clear(); + _domainInfoCache.Clear(); + _domainControllers.Clear(); lock (_uncontrolledGetDomainHintLock) { _uncontrolledGetDomainHint = null; } diff --git a/src/CommonLib/Processors/GPOLocalGroupProcessor.cs b/src/CommonLib/Processors/GPOLocalGroupProcessor.cs index 2f1212ca4..da27bfefe 100644 --- a/src/CommonLib/Processors/GPOLocalGroupProcessor.cs +++ b/src/CommonLib/Processors/GPOLocalGroupProcessor.cs @@ -30,7 +30,8 @@ public class GPOLocalGroupProcessor { private static readonly Regex ExtractRid = new(@"S-1-5-32-([0-9]{3})", RegexOptions.Compiled | RegexOptions.IgnoreCase); - private static readonly ConcurrentDictionary> GpoActionCache = new(); + private static readonly ConcurrentDictionary> GpoActionCache = + new(StringComparer.OrdinalIgnoreCase); private static readonly Dictionary ValidGroupNames = new(StringComparer.OrdinalIgnoreCase) { @@ -125,7 +126,7 @@ public async Task ReadGPOLocalGroups(string gpLink, string foreach (var rid in Enum.GetValues(typeof(LocalGroupRids))) data[(LocalGroupRids)rid] = new GroupResults(); foreach (var linkDn in orderedLinks) { - if (!GpoActionCache.TryGetValue(linkDn.ToLower(), out var actions)) { + if (!GpoActionCache.TryGetValue(linkDn, out var actions)) { actions = new List(); var gpoDomain = Helpers.DistinguishedNameToDomain(linkDn); @@ -154,7 +155,7 @@ public async Task ReadGPOLocalGroups(string gpLink, string } //Cache the actions for this GPO for later - GpoActionCache.TryAdd(linkDn.ToLower(), actions); + GpoActionCache.TryAdd(linkDn, actions); //If there are no actions, then we can move on from this GPO if (actions.Count == 0) diff --git a/test/unit/CacheTest.cs b/test/unit/CacheTest.cs index a4d040373..12cc5e8a6 100644 --- a/test/unit/CacheTest.cs +++ b/test/unit/CacheTest.cs @@ -1,4 +1,5 @@ using System; +using CommonLibTest.CollectionDefinitions; using Newtonsoft.Json; using SharpHoundCommonLib; using Xunit; @@ -6,6 +7,7 @@ namespace CommonLibTest { + [Collection(nameof(CacheTestCollectionDefinition))] public class CacheTest { private ITestOutputHelper _testOutputHelper; @@ -25,5 +27,105 @@ public void Cache_TestNewCache() Assert.Equal(cache.CacheCreationVersion, version); Assert.Equal(cache.CacheCreationDate, time); } + + [Fact] + public void AddDomainSidMapping_WritesBothDirections() + { + Cache.SetCacheInstance(Cache.CreateNewCache()); + + const string sid = "S-1-5-21-1111111111-2222222222-3333333333"; + const string name = "CONTOSO.LOCAL"; + + Cache.AddDomainSidMapping(sid, name); + + Assert.True(Cache.GetDomainSidMapping(sid, out var resolvedName)); + Assert.Equal(name, resolvedName); + Assert.True(Cache.GetDomainSidMapping(name, out var resolvedSid)); + Assert.Equal(sid, resolvedSid); + + Cache.SetCacheInstance(null); + } + + [Fact] + public void AddDomainSidMapping_BidirectionalRegardlessOfArgumentOrder() + { + Cache.SetCacheInstance(Cache.CreateNewCache()); + + const string sid = "S-1-5-21-4444444444-5555555555-6666666666"; + const string name = "FABRIKAM.LOCAL"; + + // Reverse argument order: caller passes (name, sid) instead of (sid, name). + Cache.AddDomainSidMapping(name, sid); + + Assert.True(Cache.GetDomainSidMapping(sid, out var resolvedName)); + Assert.Equal(name, resolvedName); + Assert.True(Cache.GetDomainSidMapping(name, out var resolvedSid)); + Assert.Equal(sid, resolvedSid); + + Cache.SetCacheInstance(null); + } + + [Fact] + public void AddDomainSidMapping_LookupIsCaseInsensitive() + { + Cache.SetCacheInstance(Cache.CreateNewCache()); + + const string sid = "S-1-5-21-7777777777-8888888888-9999999999"; + const string name = "CONTOSO.LOCAL"; + + Cache.AddDomainSidMapping(sid, name); + + Assert.True(Cache.GetDomainSidMapping("contoso.local", out var resolvedSid)); + Assert.Equal(sid, resolvedSid); + + Cache.SetCacheInstance(null); + } + + [Fact] + public void AddDomainSidMapping_FirstWriterWins() + { + Cache.SetCacheInstance(Cache.CreateNewCache()); + + const string sid = "S-1-5-21-1010101010-2020202020-3030303030"; + const string firstName = "FIRST.LOCAL"; + const string secondName = "SECOND.LOCAL"; + + Cache.AddDomainSidMapping(sid, firstName); + Cache.AddDomainSidMapping(sid, secondName); + + Assert.True(Cache.GetDomainSidMapping(sid, out var resolvedName)); + Assert.Equal(firstName, resolvedName); + // Reverse mapping for the first name was also written; the second name's reverse + // entry was not because TryAdd preserves the existing sid->firstName entry only, + // but secondName is itself a fresh key. + Assert.True(Cache.GetDomainSidMapping(firstName, out var resolvedSid)); + Assert.Equal(sid, resolvedSid); + + Cache.SetCacheInstance(null); + } + + [Theory] + [InlineData(null, "CONTOSO.LOCAL")] + [InlineData("S-1-5-21-1-2-3", null)] + [InlineData("", "CONTOSO.LOCAL")] + [InlineData("S-1-5-21-1-2-3", "")] + [InlineData(null, null)] + public void AddDomainSidMapping_NullOrEmptyArgumentsAreIgnored(string key, string value) + { + Cache.SetCacheInstance(Cache.CreateNewCache()); + + Cache.AddDomainSidMapping(key, value); + + if (!string.IsNullOrEmpty(key)) + { + Assert.False(Cache.GetDomainSidMapping(key, out _)); + } + if (!string.IsNullOrEmpty(value)) + { + Assert.False(Cache.GetDomainSidMapping(value, out _)); + } + + Cache.SetCacheInstance(null); + } } } From 6821eb08d9054461f6f28e4c091011612e2880dc Mon Sep 17 00:00:00 2001 From: rvazarkar Date: Tue, 28 Apr 2026 12:50:15 -0400 Subject: [PATCH 07/20] fix: rewrap cache dictionaries as case insensitive after deserialization fix: set null cache key before writes to ensure conflicts are resolved properly --- src/CommonLib/Cache.cs | 24 ++++++++++++++++ src/CommonLib/LdapUtils.cs | 25 +++++++++------- test/unit/CacheTest.cs | 59 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 98 insertions(+), 10 deletions(-) diff --git a/src/CommonLib/Cache.cs b/src/CommonLib/Cache.cs index 5bd1379df..9a0d6cf67 100644 --- a/src/CommonLib/Cache.cs +++ b/src/CommonLib/Cache.cs @@ -160,9 +160,33 @@ public static Cache CreateNewCache(Version version = null) public static void SetCacheInstance(Cache cache) { CacheInstance = cache; + NormalizeCaseInsensitiveCaches(); CreateMissingDictionaries(); } + /// + /// Rewraps dictionaries that must be case-insensitive after assignment. Serializers + /// (DataContractSerializer, Newtonsoft.Json, System.Text.Json) reconstruct + /// via its parameterless constructor, + /// which produces a case-sensitive instance regardless of how the dictionary was + /// created prior to serialization. Without this rewrap, a loaded cache silently + /// regresses the case-insensitive invariants applied at construction time. + /// + private static void NormalizeCaseInsensitiveCaches() + { + if (CacheInstance == null) return; + if (CacheInstance.SIDToDomainCache != null) + { + CacheInstance.SIDToDomainCache = new ConcurrentDictionary( + CacheInstance.SIDToDomainCache, StringComparer.OrdinalIgnoreCase); + } + if (CacheInstance.GlobalCatalogCache != null) + { + CacheInstance.GlobalCatalogCache = new ConcurrentDictionary( + CacheInstance.GlobalCatalogCache, StringComparer.OrdinalIgnoreCase); + } + } + /// /// Gets stats from the currently loaded cache /// diff --git a/src/CommonLib/LdapUtils.cs b/src/CommonLib/LdapUtils.cs index 9d32d365a..667f7a7d0 100644 --- a/src/CommonLib/LdapUtils.cs +++ b/src/CommonLib/LdapUtils.cs @@ -1254,7 +1254,10 @@ internal static DomainInfo SelectRicherDomainInfo(DomainInfo seed, DomainInfo en /// /// Resolution order: /// - /// Static cache lookup keyed by (or when null). + /// Static cache lookup keyed by the effective domain hint (the explicit + /// when supplied, otherwise the resolved current-user + /// domain via ). Falls back to + /// only when no hint can be resolved. /// Controlled LDAP resolution via the connection pool (see ). /// Uncontrolled fallback via System.DirectoryServices.ActiveDirectory.Domain.GetDomain, /// gated on (see ). @@ -1263,25 +1266,27 @@ internal static DomainInfo SelectRicherDomainInfo(DomainInfo seed, DomainInfo en /// is invoked). /// public async Task<(bool Success, DomainInfo DomainInfo)> GetDomainInfoAsync(string domainName) { - // Null domain names are stored under a per-instance sentinel to match the existing - // _domainCache null-key convention used by the legacy GetDomain overloads. - var cacheKey = domainName ?? _nullCacheKey; + // Canonicalize the cache key BEFORE the lookup. Without this, a null domainName was + // stored under a per-instance GUID sentinel, fragmenting the static _domainInfoCache: + // N LdapUtils instances each wrote a separate entry for the same effective domain, + // and a later call with the explicit DNS name missed the entry written under null. + // ResolveEffectiveDomainHint returns explicit arguments verbatim and single-shot-caches + // the uncontrolled tier in _uncontrolledGetDomainHint, so this is essentially free. + var effectiveDomain = ResolveEffectiveDomainHint(domainName); + var cacheKey = !string.IsNullOrWhiteSpace(effectiveDomain) ? effectiveDomain : _nullCacheKey; if (_domainInfoCache.TryGetValue(cacheKey, out var cached)) { return (true, cached); } // Preferred path: every LDAP call made here flows through LdapConnectionPool and - // therefore honors every flag on LdapConfig. - var (controlledOk, controlledInfo) = await ResolveDomainInfoControlledAsync(domainName); + // therefore honors every flag on LdapConfig. Pass the already-resolved hint so the + // controlled adapter does not redo ResolveEffectiveDomainHint internally. + var (controlledOk, controlledInfo) = await ResolveDomainInfoControlledAsync(effectiveDomain); if (controlledOk) { CacheDomainInfo(cacheKey, controlledInfo); return (true, controlledInfo); } - // Resolve the effective domain hint so the remaining tiers can still operate when the - // caller passed null (netonly / workgroup hosts rely on LdapConfig.CurrentUserDomain here). - var effectiveDomain = ResolveEffectiveDomainHint(domainName); - // Secondary controlled path: bind directly to the domain name with a one-shot // LdapConnection honoring LdapConfig. Tried before ADSI because this tier is the // only one that can express fine-grained LdapConfig.AuthType and diff --git a/test/unit/CacheTest.cs b/test/unit/CacheTest.cs index 12cc5e8a6..38f75518b 100644 --- a/test/unit/CacheTest.cs +++ b/test/unit/CacheTest.cs @@ -127,5 +127,64 @@ public void AddDomainSidMapping_NullOrEmptyArgumentsAreIgnored(string key, strin Cache.SetCacheInstance(null); } + + [Fact] + public void SetCacheInstance_AfterDeserialization_RestoresCaseInsensitiveSidToDomain() + { + // Simulate the load-from-disk scenario by round-tripping with + // ObjectCreationHandling.Replace, which forces the deserializer to assign a fresh + // ConcurrentDictionary via the property setter rather than reusing the one created + // by the private parameterless constructor. This reproduces the behavior of + // serializers (DataContractSerializer, System.Text.Json) that always replace via + // setters and therefore drop the OrdinalIgnoreCase comparer. + var original = Cache.CreateNewCache(); + Cache.SetCacheInstance(original); + Cache.AddDomainSidMapping("S-1-5-21-1-2-3", "CONTOSO.LOCAL"); + + var json = JsonConvert.SerializeObject(original); + var settings = new JsonSerializerSettings + { + ObjectCreationHandling = ObjectCreationHandling.Replace + }; + var deserialized = JsonConvert.DeserializeObject(json, settings); + + // Sanity: the freshly deserialized dictionary is case-sensitive - a differently + // cased key misses against the inner dict directly. Proves the test reproduces + // the regression the rewrap is fixing. + Assert.False(deserialized.SIDToDomainCache.TryGetValue("contoso.local", out _)); + + Cache.SetCacheInstance(deserialized); + + Assert.True(Cache.GetDomainSidMapping("contoso.local", out var resolvedSid)); + Assert.Equal("S-1-5-21-1-2-3", resolvedSid); + Assert.True(Cache.GetDomainSidMapping("s-1-5-21-1-2-3", out var resolvedName)); + Assert.Equal("CONTOSO.LOCAL", resolvedName); + + Cache.SetCacheInstance(null); + } + + [Fact] + public void SetCacheInstance_AfterDeserialization_RestoresCaseInsensitiveGlobalCatalog() + { + var original = Cache.CreateNewCache(); + Cache.SetCacheInstance(original); + Cache.AddGCCache("CONTOSO.LOCAL", new[] { "gc1.contoso.local", "gc2.contoso.local" }); + + var json = JsonConvert.SerializeObject(original); + var settings = new JsonSerializerSettings + { + ObjectCreationHandling = ObjectCreationHandling.Replace + }; + var deserialized = JsonConvert.DeserializeObject(json, settings); + + Assert.False(deserialized.GlobalCatalogCache.TryGetValue("contoso.local", out _)); + + Cache.SetCacheInstance(deserialized); + + Assert.True(Cache.GetGCCache("contoso.local", out var gcs)); + Assert.Equal(2, gcs.Length); + + Cache.SetCacheInstance(null); + } } } From 5de81703016db09d0452cae843eaf8a9bae7c513 Mon Sep 17 00:00:00 2001 From: rvazarkar Date: Tue, 28 Apr 2026 12:57:43 -0400 Subject: [PATCH 08/20] fix: valuetoidcache using poor casing leading to multiple cache entries for one item --- src/CommonLib/Cache.cs | 10 ++++++++-- test/unit/CacheTest.cs | 44 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 52 insertions(+), 2 deletions(-) diff --git a/src/CommonLib/Cache.cs b/src/CommonLib/Cache.cs index 9a0d6cf67..3ff4d563e 100644 --- a/src/CommonLib/Cache.cs +++ b/src/CommonLib/Cache.cs @@ -24,7 +24,7 @@ public class Cache private Cache() { - ValueToIdCache = new ConcurrentDictionary(); + ValueToIdCache = new ConcurrentDictionary(StringComparer.OrdinalIgnoreCase); IdToTypeCache = new ConcurrentDictionary(); GlobalCatalogCache = new ConcurrentDictionary(StringComparer.OrdinalIgnoreCase); MachineSidCache = new ConcurrentDictionary(); @@ -185,6 +185,11 @@ private static void NormalizeCaseInsensitiveCaches() CacheInstance.GlobalCatalogCache = new ConcurrentDictionary( CacheInstance.GlobalCatalogCache, StringComparer.OrdinalIgnoreCase); } + if (CacheInstance.ValueToIdCache != null) + { + CacheInstance.ValueToIdCache = new ConcurrentDictionary( + CacheInstance.ValueToIdCache, StringComparer.OrdinalIgnoreCase); + } } /// @@ -222,7 +227,8 @@ private static void CreateMissingDictionaries() CacheInstance.MachineSidCache ??= new ConcurrentDictionary(); CacheInstance.SIDToDomainCache ??= new ConcurrentDictionary(StringComparer.OrdinalIgnoreCase); - CacheInstance.ValueToIdCache ??= new ConcurrentDictionary(); + CacheInstance.ValueToIdCache ??= + new ConcurrentDictionary(StringComparer.OrdinalIgnoreCase); } } } \ No newline at end of file diff --git a/test/unit/CacheTest.cs b/test/unit/CacheTest.cs index 38f75518b..97814ada2 100644 --- a/test/unit/CacheTest.cs +++ b/test/unit/CacheTest.cs @@ -186,5 +186,49 @@ public void SetCacheInstance_AfterDeserialization_RestoresCaseInsensitiveGlobalC Cache.SetCacheInstance(null); } + + [Theory] + [InlineData("Administrator", "CONTOSO.LOCAL", "administrator", "contoso.local")] + [InlineData("administrator", "contoso.local", "ADMINISTRATOR", "CONTOSO.LOCAL")] + [InlineData("HOST01$", "contoso.local", "host01$", "CONTOSO.LOCAL")] + public void AddPrefixedValue_LookupIsCaseInsensitiveOnBothComponents( + string writeName, string writeDomain, string readName, string readDomain) + { + // samAccountName uses caseIgnoreString syntax in AD and DNS domain names are + // case-insensitive; the cache must reflect that to avoid fragmented entries + // and redundant LDAP queries. + Cache.SetCacheInstance(Cache.CreateNewCache()); + + Cache.AddPrefixedValue(writeName, writeDomain, "S-1-5-21-1-2-3-1001"); + + Assert.True(Cache.GetPrefixedValue(readName, readDomain, out var resolved)); + Assert.Equal("S-1-5-21-1-2-3-1001", resolved); + + Cache.SetCacheInstance(null); + } + + [Fact] + public void SetCacheInstance_AfterDeserialization_RestoresCaseInsensitiveValueToId() + { + var original = Cache.CreateNewCache(); + Cache.SetCacheInstance(original); + Cache.AddPrefixedValue("Administrator", "CONTOSO.LOCAL", "S-1-5-21-1-2-3-500"); + + var json = JsonConvert.SerializeObject(original); + var settings = new JsonSerializerSettings + { + ObjectCreationHandling = ObjectCreationHandling.Replace + }; + var deserialized = JsonConvert.DeserializeObject(json, settings); + + Assert.False(deserialized.ValueToIdCache.TryGetValue("administrator|contoso.local", out _)); + + Cache.SetCacheInstance(deserialized); + + Assert.True(Cache.GetPrefixedValue("administrator", "contoso.local", out var resolved)); + Assert.Equal("S-1-5-21-1-2-3-500", resolved); + + Cache.SetCacheInstance(null); + } } } From ca5f73e23702fc13b222220c86a73ae807cb8e98 Mon Sep 17 00:00:00 2001 From: rvazarkar Date: Tue, 28 Apr 2026 13:23:08 -0400 Subject: [PATCH 09/20] chore: fix cache misses on old GetDomain calls --- src/CommonLib/LdapUtils.cs | 146 ++++++++++++++++++++++++------------- test/unit/LDAPUtilsTest.cs | 16 ++++ 2 files changed, 113 insertions(+), 49 deletions(-) diff --git a/src/CommonLib/LdapUtils.cs b/src/CommonLib/LdapUtils.cs index 667f7a7d0..ff9743222 100644 --- a/src/CommonLib/LdapUtils.cs +++ b/src/CommonLib/LdapUtils.cs @@ -68,6 +68,13 @@ private readonly ConcurrentDictionary private readonly IPortScanner _portScanner; private readonly NativeMethods _nativeMethods; private readonly string _nullCacheKey = Guid.NewGuid().ToString(); + + // Per-instance cache for the no-hint Domain resolution. The OS-side resolution depends + // on the calling thread's auth context and/or _ldapConfig credentials, neither of which + // is a usable cache key, so this state cannot live in the static _domainCache. Failures + // are not cached; the next call retries. + private Domain _currentDomain; + private readonly object _currentDomainLock = new(); private static readonly Regex SIDRegex = new(@"^(S-\d+-\d+-\d+-\d+-\d+-\d+)(-\d+)?$"); private readonly string[] _translateNames = { "Administrator", "admin" }; @@ -510,7 +517,7 @@ public bool GetDomain(string domainName, out Domain domain) { domain = null; return false; } - + if (!string.IsNullOrWhiteSpace(_ldapConfig.Server)) { _log.LogDebug( "GetDomain(\"{Name}\", out Domain) short-circuited: Specific Server is set", @@ -519,26 +526,33 @@ public bool GetDomain(string domainName, out Domain domain) { return false; } - var cacheKey = domainName ?? _nullCacheKey; - if (_domainCache.TryGetValue(cacheKey, out domain)) return true; + // A blank/whitespace name is the no-target form. Delegate so the per-instance + // _currentDomain handles it without a per-instance GUID sentinel against the + // process-wide static cache. + if (string.IsNullOrWhiteSpace(domainName)) { + return GetDomain(out domain); + } + + if (_domainCache.TryGetValue(domainName, out domain)) return true; try { - DirectoryContext context; - if (_ldapConfig.Username != null) - context = domainName != null - ? new DirectoryContext(DirectoryContextType.Domain, domainName, _ldapConfig.Username, - _ldapConfig.Password) - : new DirectoryContext(DirectoryContextType.Domain, _ldapConfig.Username, - _ldapConfig.Password); - else - context = domainName != null - ? new DirectoryContext(DirectoryContextType.Domain, domainName) - : new DirectoryContext(DirectoryContextType.Domain); + var context = _ldapConfig.Username != null + ? new DirectoryContext(DirectoryContextType.Domain, domainName, _ldapConfig.Username, + _ldapConfig.Password) + : new DirectoryContext(DirectoryContextType.Domain, domainName); // Blocking External Call domain = Domain.GetDomain(context); if (domain == null) return false; - _domainCache.TryAdd(cacheKey, domain); + + // Cache under the caller's input AND the canonical name returned by SDS. The + // two may differ (e.g. NetBIOS alias vs DNS canonical), so writing both keys + // ensures alias-form callers don't miss while future canonical-form callers + // still benefit. + _domainCache.TryAdd(domainName, domain); + if (!string.IsNullOrWhiteSpace(domain.Name)) { + _domainCache.TryAdd(domain.Name, domain); + } return true; } catch (Exception e) { @@ -556,34 +570,41 @@ public static bool GetDomain(string domainName, LdapConfig ldapConfig, out Domai domain = null; return false; } - + if (!string.IsNullOrWhiteSpace(ldapConfig.Server)) { Logging.Logger.LogDebug( - "GetDomain(\"{Name}\", out Domain) short-circuited: Specific Server is set", + "Static GetDomain(\"{Name}\", out Domain) short-circuited: Specific Server is set", domainName); domain = null; return false; } + // The static overload has no per-instance state to cache a no-hint resolution + // against, and ConcurrentDictionary throws on a null key. Reject up front rather + // than proceeding with an unkeyable resolution. + if (string.IsNullOrWhiteSpace(domainName)) { + Logging.Logger.LogDebug( + "Static GetDomain short-circuited: domainName is null or whitespace"); + domain = null; + return false; + } + if (_domainCache.TryGetValue(domainName, out domain)) return true; try { - DirectoryContext context; - if (ldapConfig.Username != null) - context = domainName != null - ? new DirectoryContext(DirectoryContextType.Domain, domainName, ldapConfig.Username, - ldapConfig.Password) - : new DirectoryContext(DirectoryContextType.Domain, ldapConfig.Username, - ldapConfig.Password); - else - context = domainName != null - ? new DirectoryContext(DirectoryContextType.Domain, domainName) - : new DirectoryContext(DirectoryContextType.Domain); + var context = ldapConfig.Username != null + ? new DirectoryContext(DirectoryContextType.Domain, domainName, ldapConfig.Username, + ldapConfig.Password) + : new DirectoryContext(DirectoryContextType.Domain, domainName); // Blocking External Call domain = Domain.GetDomain(context); if (domain == null) return false; + _domainCache.TryAdd(domainName, domain); + if (!string.IsNullOrWhiteSpace(domain.Name)) { + _domainCache.TryAdd(domain.Name, domain); + } return true; } catch (Exception e) { @@ -595,12 +616,12 @@ public static bool GetDomain(string domainName, LdapConfig ldapConfig, out Domai } /// - /// Attempts to get the Domain object representing the target domain. If null is specified for the domain name, gets - /// the user's current domain + /// Attempts to get the Domain object representing the user's current domain. The + /// resolution depends on the calling thread's auth context and configured credentials, + /// so the result is cached per instance rather than in the + /// shared static cache. Successful resolutions also seed the static cache under the + /// canonical DNS name so that future explicit-name callers benefit. /// - /// - /// - /// public bool GetDomain(out Domain domain) { if (!_ldapConfig.AllowFallbackToUncontrolledLdap) { _log.LogDebug( @@ -608,7 +629,7 @@ public bool GetDomain(out Domain domain) { domain = null; return false; } - + if (!string.IsNullOrWhiteSpace(_ldapConfig.Server)) { _log.LogDebug( "GetDomain() short-circuited: Specific Server is set"); @@ -616,23 +637,47 @@ public bool GetDomain(out Domain domain) { return false; } - if (_domainCache.TryGetValue(_nullCacheKey, out domain)) return true; - - try { - var context = _ldapConfig.Username != null - ? new DirectoryContext(DirectoryContextType.Domain, _ldapConfig.Username, - _ldapConfig.Password) - : new DirectoryContext(DirectoryContextType.Domain); - - // Blocking External Call - domain = Domain.GetDomain(context); - _domainCache.TryAdd(_nullCacheKey, domain); + // Lock-free fast path for the common case of repeated null-hint calls on a single + // instance after the first successful resolution. + if (_currentDomain != null) { + domain = _currentDomain; return true; } - catch (Exception e) { - _log.LogDebug(e, "GetDomain call failed for blank domain"); - domain = null; - return false; + + // Serialize concurrent first-time resolutions on the same instance so we don't fan + // out duplicate Domain.GetDomain RPCs. + lock (_currentDomainLock) { + if (_currentDomain != null) { + domain = _currentDomain; + return true; + } + + try { + var context = _ldapConfig.Username != null + ? new DirectoryContext(DirectoryContextType.Domain, _ldapConfig.Username, + _ldapConfig.Password) + : new DirectoryContext(DirectoryContextType.Domain); + + // Blocking External Call + var resolved = Domain.GetDomain(context); + if (resolved == null) { + domain = null; + return false; + } + + _currentDomain = resolved; + if (!string.IsNullOrWhiteSpace(resolved.Name)) { + _domainCache.TryAdd(resolved.Name, resolved); + } + + domain = resolved; + return true; + } + catch (Exception e) { + _log.LogDebug(e, "GetDomain call failed for blank domain"); + domain = null; + return false; + } } } @@ -2327,6 +2372,9 @@ public void ResetUtils() { _domainCache.Clear(); _domainInfoCache.Clear(); _domainControllers.Clear(); + lock (_currentDomainLock) { + _currentDomain = null; + } lock (_uncontrolledGetDomainHintLock) { _uncontrolledGetDomainHint = null; } diff --git a/test/unit/LDAPUtilsTest.cs b/test/unit/LDAPUtilsTest.cs index a14f411d7..6f2321c35 100644 --- a/test/unit/LDAPUtilsTest.cs +++ b/test/unit/LDAPUtilsTest.cs @@ -711,6 +711,22 @@ public void GetDomain_OutDomain_ReturnsFalse_WhenFallbackDisabled() { Assert.Null(staticDomain); } + [Theory] + [InlineData(null)] + [InlineData("")] + [InlineData(" ")] + public void GetDomain_Static_ReturnsFalse_WithoutThrowing_OnBlankName(string domainName) { + // ConcurrentDictionary throws on null keys, so the static overload would previously + // crash on a null hint. The static has no per-instance null-resolution cache to fall + // back on, so blank inputs are rejected up front. + var config = new LdapConfig { AllowFallbackToUncontrolledLdap = true }; + + var success = LdapUtils.GetDomain(domainName, config, out var domain); + + Assert.False(success); + Assert.Null(domain); + } + // --------------------------------------------------------------------------- // TryStripNtdsSettingsPrefix // --------------------------------------------------------------------------- From a90d8a3b6dce0952543e5c16a292f24f55535a65 Mon Sep 17 00:00:00 2001 From: rvazarkar Date: Tue, 28 Apr 2026 13:30:35 -0400 Subject: [PATCH 10/20] chore: properly dispose domain objects --- src/CommonLib/LdapUtils.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/CommonLib/LdapUtils.cs b/src/CommonLib/LdapUtils.cs index ff9743222..5e3769123 100644 --- a/src/CommonLib/LdapUtils.cs +++ b/src/CommonLib/LdapUtils.cs @@ -1849,7 +1849,7 @@ private static bool TryGetDomainInfoViaUncontrolledFallback(string domainName, L : new DirectoryContext(DirectoryContextType.Domain); // Blocking External Call - var domain = Domain.GetDomain(context); + using var domain = Domain.GetDomain(context); if (domain == null) { return false; } @@ -1907,7 +1907,7 @@ private static bool TryGetDomainInfoViaUncontrolledFallback(string domainName, L // (matching Helpers.CreateDirectoryEntry) before the first property access forces // the bind. try { - var rawEntry = domain.GetDirectoryEntry(); + using var rawEntry = domain.GetDirectoryEntry(); var authType = AuthenticationTypes.Secure; if (config.ForceSSL) { authType |= AuthenticationTypes.SecureSocketsLayer; From 6ba0ef47b0830e05bde8b7a68bae9ca86a1e9c2c Mon Sep 17 00:00:00 2001 From: rvazarkar Date: Tue, 28 Apr 2026 15:48:20 -0400 Subject: [PATCH 11/20] chore: significantly simplify caching and domain resolution code --- src/CommonLib/LdapUtils.cs | 278 ++++++++++++------------------------- test/unit/LDAPUtilsTest.cs | 115 +++++++++++++++ 2 files changed, 207 insertions(+), 186 deletions(-) diff --git a/src/CommonLib/LdapUtils.cs b/src/CommonLib/LdapUtils.cs index 5e3769123..d09d61ba7 100644 --- a/src/CommonLib/LdapUtils.cs +++ b/src/CommonLib/LdapUtils.cs @@ -30,17 +30,11 @@ namespace SharpHoundCommonLib { public class LdapUtils : ILdapUtils { - //This cache is indexed by domain sid - private static ConcurrentDictionary _domainCache = - new(StringComparer.OrdinalIgnoreCase); private static ConcurrentDictionary _domainInfoCache = new(StringComparer.OrdinalIgnoreCase); private static ConcurrentHashSet _domainControllers = new(StringComparer.OrdinalIgnoreCase); private static ConcurrentHashSet _unresolvablePrincipals = new(StringComparer.OrdinalIgnoreCase); - private static readonly ConcurrentDictionary DomainToForestCache = - new(StringComparer.OrdinalIgnoreCase); - // Single-shot cache for the uncontrolled Domain.GetDomain() hint tier in // ResolveEffectiveDomainHint. null = not attempted; "" = attempted and failed; otherwise the // resolved domain name. Guarded by _uncontrolledGetDomainHintLock because the underlying RPC @@ -67,12 +61,11 @@ private readonly ConcurrentDictionary private readonly ILogger _log; private readonly IPortScanner _portScanner; private readonly NativeMethods _nativeMethods; - private readonly string _nullCacheKey = Guid.NewGuid().ToString(); // Per-instance cache for the no-hint Domain resolution. The OS-side resolution depends // on the calling thread's auth context and/or _ldapConfig credentials, neither of which - // is a usable cache key, so this state cannot live in the static _domainCache. Failures - // are not cached; the next call retries. + // is a usable cache key for a process-wide store. Failures are not cached; the next call + // retries. private Domain _currentDomain; private readonly object _currentDomainLock = new(); private static readonly Regex SIDRegex = new(@"^(S-\d+-\d+-\d+-\d+-\d+-\d+)(-\d+)?$"); @@ -316,23 +309,14 @@ public IAsyncEnumerable> PagedQuery(LdapQueryParame } public virtual async Task<(bool Success, string ForestName)> GetForest(string domain) { - if (DomainToForestCache.TryGetValue(domain, out var cachedForest)) { - return (true, cachedForest); - } - + // DomainInfo.ForestName is already memoized in _domainInfoCache, so a separate + // domain-to-forest dictionary would be a duplicate cache over the same key space. if (await GetDomainInfoAsync(domain) is (true, var domainInfo) && !string.IsNullOrEmpty(domainInfo?.ForestName)) { - DomainToForestCache.TryAdd(domain, domainInfo.ForestName); return (true, domainInfo.ForestName); } - var (success, forest) = await GetForestFromLdap(domain); - if (success) { - DomainToForestCache.TryAdd(domain, forest); - return (true, forest); - } - - return (false, null); + return await GetForestFromLdap(domain); } private async Task<(bool Success, string ForestName)> GetForestFromLdap(string domain) { @@ -527,14 +511,11 @@ public bool GetDomain(string domainName, out Domain domain) { } // A blank/whitespace name is the no-target form. Delegate so the per-instance - // _currentDomain handles it without a per-instance GUID sentinel against the - // process-wide static cache. + // _currentDomain handles it. if (string.IsNullOrWhiteSpace(domainName)) { return GetDomain(out domain); } - if (_domainCache.TryGetValue(domainName, out domain)) return true; - try { var context = _ldapConfig.Username != null ? new DirectoryContext(DirectoryContextType.Domain, domainName, _ldapConfig.Username, @@ -543,17 +524,7 @@ public bool GetDomain(string domainName, out Domain domain) { // Blocking External Call domain = Domain.GetDomain(context); - if (domain == null) return false; - - // Cache under the caller's input AND the canonical name returned by SDS. The - // two may differ (e.g. NetBIOS alias vs DNS canonical), so writing both keys - // ensures alias-form callers don't miss while future canonical-form callers - // still benefit. - _domainCache.TryAdd(domainName, domain); - if (!string.IsNullOrWhiteSpace(domain.Name)) { - _domainCache.TryAdd(domain.Name, domain); - } - return true; + return domain != null; } catch (Exception e) { _log.LogDebug(e, "GetDomain call failed for domain name {Name}", domainName); @@ -579,9 +550,8 @@ public static bool GetDomain(string domainName, LdapConfig ldapConfig, out Domai return false; } - // The static overload has no per-instance state to cache a no-hint resolution - // against, and ConcurrentDictionary throws on a null key. Reject up front rather - // than proceeding with an unkeyable resolution. + // The static overload has no per-instance state to anchor a no-hint resolution to. + // Reject up front rather than proceeding with a resolution we can't attribute. if (string.IsNullOrWhiteSpace(domainName)) { Logging.Logger.LogDebug( "Static GetDomain short-circuited: domainName is null or whitespace"); @@ -589,8 +559,6 @@ public static bool GetDomain(string domainName, LdapConfig ldapConfig, out Domai return false; } - if (_domainCache.TryGetValue(domainName, out domain)) return true; - try { var context = ldapConfig.Username != null ? new DirectoryContext(DirectoryContextType.Domain, domainName, ldapConfig.Username, @@ -599,13 +567,7 @@ public static bool GetDomain(string domainName, LdapConfig ldapConfig, out Domai // Blocking External Call domain = Domain.GetDomain(context); - if (domain == null) return false; - - _domainCache.TryAdd(domainName, domain); - if (!string.IsNullOrWhiteSpace(domain.Name)) { - _domainCache.TryAdd(domain.Name, domain); - } - return true; + return domain != null; } catch (Exception e) { Logging.Logger.LogDebug("Static GetDomain call failed for domain {DomainName}: {Error}", domainName, @@ -618,9 +580,7 @@ public static bool GetDomain(string domainName, LdapConfig ldapConfig, out Domai /// /// Attempts to get the Domain object representing the user's current domain. The /// resolution depends on the calling thread's auth context and configured credentials, - /// so the result is cached per instance rather than in the - /// shared static cache. Successful resolutions also seed the static cache under the - /// canonical DNS name so that future explicit-name callers benefit. + /// so the result is cached per instance. /// public bool GetDomain(out Domain domain) { if (!_ldapConfig.AllowFallbackToUncontrolledLdap) { @@ -660,16 +620,8 @@ public bool GetDomain(out Domain domain) { // Blocking External Call var resolved = Domain.GetDomain(context); - if (resolved == null) { - domain = null; - return false; - } _currentDomain = resolved; - if (!string.IsNullOrWhiteSpace(resolved.Name)) { - _domainCache.TryAdd(resolved.Name, resolved); - } - domain = resolved; return true; } @@ -1252,17 +1204,47 @@ internal static int CompletenessScore(DomainInfo info) { /// CN=Partitions for . Because the static helper /// is invoked re-entrantly from /// during pool acquisition, the sparser one-shot direct-LDAP record reaches the cache first; - /// without this helper the subsequent pool-derived record would be silently discarded by + /// without the score guard the subsequent pool-derived record would be silently discarded by /// TryAdd and every later cache hit would observe the partial record. + /// + /// The cross-domain guard () prevents cache poisoning when + /// a misconfigured pin or a cross-forest leak causes a tier + /// to return a describing a different domain than the one the + /// caller asked about. On every successful write the entry is also mirrored under + /// when that differs from , so + /// subsequent lookups by a different alias form (NetBIOS short name vs. DNS FQDN) hit the + /// same richest-available record. + /// /// internal static void CacheDomainInfo(string key, DomainInfo candidate) { if (key == null || candidate == null) return; - _domainInfoCache.AddOrUpdate( - key, - candidate, - (_, existing) => CompletenessScore(candidate) > CompletenessScore(existing) - ? candidate - : existing); + if (!KeyMatchesCandidate(key, candidate)) return; + + DomainInfo PickRicher(string _, DomainInfo existing) + => CompletenessScore(candidate) > CompletenessScore(existing) ? candidate : existing; + + _domainInfoCache.AddOrUpdate(key, candidate, PickRicher); + + if (!string.IsNullOrWhiteSpace(candidate.Name) + && !string.Equals(key, candidate.Name, StringComparison.OrdinalIgnoreCase)) { + _domainInfoCache.AddOrUpdate(candidate.Name, candidate, PickRicher); + } + } + + /// + /// Returns true when identifies the same domain that + /// describes, comparing case-insensitively against + /// and . A candidate + /// with no is treated as unverifiable and accepted; the + /// resolution tiers always populate Name on success, so this branch only matters for + /// hand-built records in tests. + /// + private static bool KeyMatchesCandidate(string key, DomainInfo candidate) { + if (string.IsNullOrEmpty(candidate.Name)) return true; + if (string.Equals(key, candidate.Name, StringComparison.OrdinalIgnoreCase)) return true; + if (!string.IsNullOrEmpty(candidate.NetBiosName) + && string.Equals(key, candidate.NetBiosName, StringComparison.OrdinalIgnoreCase)) return true; + return false; } /// @@ -1297,87 +1279,15 @@ internal static DomainInfo SelectRicherDomainInfo(DomainInfo seed, DomainInfo en /// AuthType, signing, cert verification, credentials). /// /// - /// Resolution order: - /// - /// Static cache lookup keyed by the effective domain hint (the explicit - /// when supplied, otherwise the resolved current-user - /// domain via ). Falls back to - /// only when no hint can be resolved. - /// Controlled LDAP resolution via the connection pool (see ). - /// Uncontrolled fallback via System.DirectoryServices.ActiveDirectory.Domain.GetDomain, - /// gated on (see ). - /// - /// Successful results from either path are cached for the lifetime of the process (until - /// is invoked). + /// Walks with this instance's connection pool and + /// config, after substituting a current-user domain hint via + /// when is empty. + /// Successful results are cached in the static for the + /// lifetime of the process (until is invoked). /// - public async Task<(bool Success, DomainInfo DomainInfo)> GetDomainInfoAsync(string domainName) { - // Canonicalize the cache key BEFORE the lookup. Without this, a null domainName was - // stored under a per-instance GUID sentinel, fragmenting the static _domainInfoCache: - // N LdapUtils instances each wrote a separate entry for the same effective domain, - // and a later call with the explicit DNS name missed the entry written under null. - // ResolveEffectiveDomainHint returns explicit arguments verbatim and single-shot-caches - // the uncontrolled tier in _uncontrolledGetDomainHint, so this is essentially free. - var effectiveDomain = ResolveEffectiveDomainHint(domainName); - var cacheKey = !string.IsNullOrWhiteSpace(effectiveDomain) ? effectiveDomain : _nullCacheKey; - if (_domainInfoCache.TryGetValue(cacheKey, out var cached)) { - return (true, cached); - } - - // Preferred path: every LDAP call made here flows through LdapConnectionPool and - // therefore honors every flag on LdapConfig. Pass the already-resolved hint so the - // controlled adapter does not redo ResolveEffectiveDomainHint internally. - var (controlledOk, controlledInfo) = await ResolveDomainInfoControlledAsync(effectiveDomain); - if (controlledOk) { - CacheDomainInfo(cacheKey, controlledInfo); - return (true, controlledInfo); - } - - // Secondary controlled path: bind directly to the domain name with a one-shot - // LdapConnection honoring LdapConfig. Tried before ADSI because this tier is the - // only one that can express fine-grained LdapConfig.AuthType and - // LdapConfig.DisableCertVerification - running ADSI first would silently ignore - // those flags whenever the ADSI bind happened to succeed. - if (!string.IsNullOrWhiteSpace(effectiveDomain)) { - var (directOk, directInfo) = - await TryResolveDomainInfoViaDirectLdapAsync(effectiveDomain, _ldapConfig, _log); - if (directOk) { - CacheDomainInfo(cacheKey, directInfo); - return (true, directInfo); - } - } - - // Tertiary controlled path: ADSI. DirectoryEntry's serverless binding invokes - // DsGetDcName internally, so NetBIOS short names and domains without a direct DNS - // A record still bind here after the raw-LDAP tier above could not resolve them. - if (!string.IsNullOrWhiteSpace(effectiveDomain)) { - var (adsiOk, adsiInfo) = - await TryResolveDomainInfoViaDirectoryEntryAsync(effectiveDomain, _ldapConfig, _log); - if (adsiOk) { - // ADSI populates PrimaryDomainController via DsGetDcName. Retrying the - // direct-LDAP path against that discovered DC closes the score gap when the - // original raw-LDAP attempt failed only because the domain name itself had - // no resolvable A/SRV record from this host. - adsiInfo = await TryEnrichDomainInfoViaDirectLdapAsync( - effectiveDomain, adsiInfo, _ldapConfig, _log); - CacheDomainInfo(cacheKey, adsiInfo); - return (true, adsiInfo); - } - } - - // Opt-in fallback. TryGetDomainInfoViaUncontrolledFallback short-circuits when - // AllowFallbackToUncontrolledLdap is disabled, so when the flag is off this call is a no-op. - if (TryGetDomainInfoViaUncontrolledFallback(effectiveDomain, _ldapConfig, _log, out var fallbackInfo)) { - // Same enrichment opportunity as the ADSI tier - the uncontrolled fallback - // populates both PrimaryDomainController and DomainControllers but never - // NetBiosName, so the discovered DC name is enough to reach a score-7 record - // via a controlled retry. - fallbackInfo = await TryEnrichDomainInfoViaDirectLdapAsync( - effectiveDomain, fallbackInfo, _ldapConfig, _log); - CacheDomainInfo(cacheKey, fallbackInfo); - return (true, fallbackInfo); - } - - return (false, null); + public Task<(bool Success, DomainInfo DomainInfo)> GetDomainInfoAsync(string domainName) { + var hint = ResolveEffectiveDomainHint(domainName); + return ResolveDomainInfoAsync(hint, _connectionPool, _ldapConfig, _log); } /// @@ -1398,14 +1308,35 @@ internal static DomainInfo SelectRicherDomainInfo(DomainInfo seed, DomainInfo en /// Config used to honor the gate and to build fallback credentials. /// Logger used for debug-level diagnostics on failure paths. /// - /// Results from both paths are stored in the same static - /// used by the instance overload, so a successful lookup here also benefits later - /// calls on the same process. + /// Results are stored in the same static used by the + /// instance overload, so a successful lookup here also benefits later + /// calls on the same process. The pool tier is + /// skipped here because no is available; the remaining + /// three tiers (one-shot direct LDAP, ADSI, uncontrolled fallback) still run. /// - internal static async Task<(bool Success, DomainInfo DomainInfo)> GetDomainInfoStaticAsync( + internal static Task<(bool Success, DomainInfo DomainInfo)> GetDomainInfoStaticAsync( string domainName, LdapConfig config, ILogger log = null) { - // Unlike the instance overload, the static entry point does not fabricate a current-domain - // hint - its callers always know which domain they are asking about. + return ResolveDomainInfoAsync(domainName, pool: null, config, log); + } + + /// + /// Shared resolution core that drives the four-tier walk for both + /// and . + /// Tier 1 (pool-driven controlled LDAP) is skipped when is null. + /// Tiers 2-4 (one-shot direct LDAP, ADSI, uncontrolled fallback) run unconditionally. + /// + /// + /// Tier ordering rationale: the pool tier honors every flag and + /// is preferred when available. The one-shot direct-LDAP tier is tried before ADSI because + /// it is the only tier outside the pool that can express fine-grained + /// and - + /// running ADSI first would silently ignore those flags whenever its serverless bind + /// happened to succeed. The ADSI and uncontrolled-fallback tiers are followed by a + /// direct-LDAP enrichment pass against the discovered PDC so the cached record reaches the + /// same shape as the pool and one-shot tiers. + /// + private static async Task<(bool Success, DomainInfo DomainInfo)> ResolveDomainInfoAsync( + string domainName, ConnectionPoolManager pool, LdapConfig config, ILogger log) { if (string.IsNullOrWhiteSpace(domainName)) { return (false, null); } @@ -1414,25 +1345,23 @@ internal static DomainInfo SelectRicherDomainInfo(DomainInfo seed, DomainInfo en return (true, cached); } - // Secondary controlled path: a one-shot LdapConnection bound directly to the domain - // name, honoring LdapConfig. Tried before ADSI because this tier is the only one - // that can express fine-grained LdapConfig.AuthType and LdapConfig.DisableCertVerification. + if (pool != null) { + var (poolOk, poolInfo) = await ResolveDomainInfoControlledAsyncCore(domainName, pool, log); + if (poolOk) { + CacheDomainInfo(domainName, poolInfo); + return (true, poolInfo); + } + } + var (directOk, directInfo) = await TryResolveDomainInfoViaDirectLdapAsync(domainName, config, log); if (directOk) { CacheDomainInfo(domainName, directInfo); return (true, directInfo); } - // Tertiary controlled path: ADSI. DirectoryEntry's serverless binding handles - // NetBIOS short names and domains whose DNS layout does not expose an A record on - // the domain name itself, at the cost of not honoring every LdapConfig flag. var (adsiOk, adsiInfo) = await TryResolveDomainInfoViaDirectoryEntryAsync(domainName, config, log); if (adsiOk) { - // Retry the direct-LDAP path against the DC discovered by ADSI so the cached - // record reaches the same shape as the pool/one-shot tiers. Mirrors the matching - // enrichment in the instance overload. - adsiInfo = await TryEnrichDomainInfoViaDirectLdapAsync( - domainName, adsiInfo, config, log); + adsiInfo = await TryEnrichDomainInfoViaDirectLdapAsync(domainName, adsiInfo, config, log); CacheDomainInfo(domainName, adsiInfo); return (true, adsiInfo); } @@ -1447,28 +1376,6 @@ internal static DomainInfo SelectRicherDomainInfo(DomainInfo seed, DomainInfo en return (false, null); } - /// - /// Instance-level adapter over that supplies - /// this utils' connection pool and logger, and substitutes a current-domain hint when - /// is null/empty. - /// - /// - /// The hint is taken from when set, otherwise - /// from . The config override is the only workable - /// hint in netonly contexts (runas /netonly) and on workgroup machines, where the - /// OS API reports the local machine name rather than the target AD domain. If neither is - /// usable the connection pool will fail to resolve the hint and this method returns - /// (false, null) so the orchestrator can fall through to the uncontrolled fallback. - /// - private Task<(bool Success, DomainInfo DomainInfo)> ResolveDomainInfoControlledAsync(string domainName) { - var hint = ResolveEffectiveDomainHint(domainName); - if (string.IsNullOrWhiteSpace(hint)) { - return Task.FromResult<(bool, DomainInfo)>((false, null)); - } - - return ResolveDomainInfoControlledAsyncCore(hint, _connectionPool, _log); - } - /// /// Resolves the domain name to use when the caller did not supply one. Walks a five-step /// preference order designed to maximize resolution success without paying an RPC on the @@ -2369,7 +2276,6 @@ private static (bool Success, DomainInfo DomainInfo) ResolveDomainInfoFromConnec public void ResetUtils() { _unresolvablePrincipals.Clear(); - _domainCache.Clear(); _domainInfoCache.Clear(); _domainControllers.Clear(); lock (_currentDomainLock) { diff --git a/test/unit/LDAPUtilsTest.cs b/test/unit/LDAPUtilsTest.cs index 6f2321c35..b9f56391e 100644 --- a/test/unit/LDAPUtilsTest.cs +++ b/test/unit/LDAPUtilsTest.cs @@ -946,6 +946,121 @@ public async Task CacheDomainInfo_NullKeyOrCandidate_NoOp() { Assert.Null(cached); } + [Fact] + public async Task CacheDomainInfo_KeyMismatchesCandidateName_NoOp() { + new LdapUtils().ResetUtils(); + const string key = "h2-contoso.test"; + + // Simulates a misconfigured LdapConfig.Server pin that lands on a DC outside the + // requested domain - the candidate's Name describes fabrikam, but the caller asked + // about contoso. The guard must reject the write to prevent cache poisoning. + var fabrikam = new DomainInfo( + name: "H2-FABRIKAM.TEST", + distinguishedName: "DC=h2-fabrikam,DC=test", + domainSid: "S-1-5-21-9-9-9", + netBiosName: "FABRIKAM"); + LdapUtils.CacheDomainInfo(key, fabrikam); + + var (ok, cached) = await LdapUtils.GetDomainInfoStaticAsync(key, new LdapConfig { + Server = "unreachable.invalid.test", + AllowFallbackToUncontrolledLdap = false + }); + Assert.False(ok); + Assert.Null(cached); + } + + [Fact] + public async Task CacheDomainInfo_KeyMatchesNetBiosName_Cached() { + new LdapUtils().ResetUtils(); + const string netBiosKey = "H2NETBIOS"; + + // NetBIOS short-name keys are a legitimate alias form - the guard must accept the + // write when key matches candidate.NetBiosName even though it differs from Name. + var info = new DomainInfo( + name: "H2-NETBIOS-MATCH.TEST", + distinguishedName: "DC=h2-netbios-match,DC=test", + domainSid: "S-1-5-21-1-1-1", + netBiosName: "H2NETBIOS"); + LdapUtils.CacheDomainInfo(netBiosKey, info); + + var (ok, cached) = await LdapUtils.GetDomainInfoStaticAsync(netBiosKey, new LdapConfig { + Server = "unreachable.invalid.test", + AllowFallbackToUncontrolledLdap = false + }); + Assert.True(ok); + Assert.Equal("H2-NETBIOS-MATCH.TEST", cached.Name); + } + + [Fact] + public async Task CacheDomainInfo_KeyMatchesCandidateNameCaseInsensitive_Cached() { + new LdapUtils().ResetUtils(); + const string lowerKey = "h2-case.test"; + + var info = new DomainInfo(name: "H2-CASE.TEST", domainSid: "S-1-5-21-2-2-2"); + LdapUtils.CacheDomainInfo(lowerKey, info); + + var (ok, cached) = await LdapUtils.GetDomainInfoStaticAsync(lowerKey, new LdapConfig { + Server = "unreachable.invalid.test", + AllowFallbackToUncontrolledLdap = false + }); + Assert.True(ok); + Assert.Equal("S-1-5-21-2-2-2", cached.DomainSid); + } + + [Fact] + public async Task CacheDomainInfo_MirrorsEntryUnderCandidateName() { + new LdapUtils().ResetUtils(); + const string netBiosKey = "H1MIRROR"; + + // Write under the NetBIOS alias - the helper must mirror the entry under the + // canonical DNS Name so a later lookup by FQDN hits the same record without redoing + // resolution. + var info = new DomainInfo( + name: "H1-MIRROR.TEST", + distinguishedName: "DC=h1-mirror,DC=test", + domainSid: "S-1-5-21-3-3-3", + netBiosName: "H1MIRROR"); + LdapUtils.CacheDomainInfo(netBiosKey, info); + + var (ok, cached) = await LdapUtils.GetDomainInfoStaticAsync("H1-MIRROR.TEST", new LdapConfig { + Server = "unreachable.invalid.test", + AllowFallbackToUncontrolledLdap = false + }); + Assert.True(ok); + Assert.Equal("H1MIRROR", cached.NetBiosName); + Assert.Equal("S-1-5-21-3-3-3", cached.DomainSid); + } + + [Fact] + public async Task CacheDomainInfo_RicherAliasWriteUpgradesNameKeyEntry() { + new LdapUtils().ResetUtils(); + const string fqdnKey = "h1-upgrade.test"; + const string netBiosKey = "H1UPGRADE"; + + // Pre-seed the canonical-Name entry with a sparse record (e.g. an earlier ADSI tier + // result). A later richer write under the NetBIOS alias must propagate through the + // mirror so a FQDN lookup observes the upgrade rather than the stale sparse entry. + var sparse = new DomainInfo(name: "H1-UPGRADE.TEST", domainSid: "S-1-5-21-4-4-4"); + LdapUtils.CacheDomainInfo(fqdnKey, sparse); + + var rich = new DomainInfo( + name: "H1-UPGRADE.TEST", + distinguishedName: "DC=h1-upgrade,DC=test", + domainSid: "S-1-5-21-4-4-4", + netBiosName: "H1UPGRADE", + primaryDomainController: "dc01.h1-upgrade.test", + domainControllers: new[] { "dc01.h1-upgrade.test" }); + LdapUtils.CacheDomainInfo(netBiosKey, rich); + + var (ok, cached) = await LdapUtils.GetDomainInfoStaticAsync(fqdnKey, new LdapConfig { + Server = "unreachable.invalid.test", + AllowFallbackToUncontrolledLdap = false + }); + Assert.True(ok); + Assert.Equal("H1UPGRADE", cached.NetBiosName); + Assert.Equal("dc01.h1-upgrade.test", cached.PrimaryDomainController); + } + // --------------------------------------------------------------------------- // SelectRicherDomainInfo / TryEnrichDomainInfoViaDirectLdapAsync coverage // --------------------------------------------------------------------------- From 9c3f16c0a331654a4ace74342b0d6afc02bd68f5 Mon Sep 17 00:00:00 2001 From: rvazarkar Date: Tue, 28 Apr 2026 16:11:54 -0400 Subject: [PATCH 12/20] fix: change sid/domain cache mapping so we dont accidentally poison the cache with netbios names --- src/CommonLib/Cache.cs | 45 ++++++++++++++---- src/CommonLib/ConnectionPoolManager.cs | 7 +++ src/CommonLib/LdapUtils.cs | 8 ++++ test/unit/CacheTest.cs | 65 ++++++++++++++++++++++++++ 4 files changed, 116 insertions(+), 9 deletions(-) diff --git a/src/CommonLib/Cache.cs b/src/CommonLib/Cache.cs index 3ff4d563e..e1f43d456 100644 --- a/src/CommonLib/Cache.cs +++ b/src/CommonLib/Cache.cs @@ -46,10 +46,12 @@ private Cache() [IgnoreDataMember] private static Cache CacheInstance { get; set; } /// - /// Add a SID/Domain-name pair to the cache. Writes both directions (SID→Name and - /// Name→SID) so a successful resolution by any tier benefits subsequent lookups in - /// either direction. Existing entries are preserved (TryAdd semantics) — first - /// resolver wins. + /// Add a SID/Domain-name pair to the cache. The Name→SID direction is always written + /// (NetBIOS aliases are valid lookup keys). The SID→Name direction is only written + /// when the name is a DNS-shaped FQDN: a NetBIOS-keyed reverse write would poison + /// the slot for downstream consumers that depend on the SID→Name lookup yielding a + /// DNS name (LDAP base DN construction, server selection, GetDomainInfoAsync hints). + /// Existing entries are preserved (TryAdd semantics) — first resolver wins. /// /// A SID or a domain name. /// The corresponding domain name or SID. @@ -57,11 +59,36 @@ internal static void AddDomainSidMapping(string key, string value) { if (CacheInstance == null) return; if (string.IsNullOrEmpty(key) || string.IsNullOrEmpty(value)) return; - CacheInstance.SIDToDomainCache.TryAdd(key, value); - // Bidirectional: SIDs (S-…) and DNS domain names cannot collide as keys, so writing - // the reverse mapping makes a successful resolution by any tier visible to lookups - // in either direction. - CacheInstance.SIDToDomainCache.TryAdd(value, key); + + var keyIsSid = LooksLikeDomainSid(key); + var valueIsSid = LooksLikeDomainSid(value); + + if (keyIsSid == valueIsSid) + { + // Both look like SIDs or neither does — caller misuse or an unexpected input + // shape. Throw this data out + return; + } + + var sid = keyIsSid ? key : value; + var name = keyIsSid ? value : key; + + CacheInstance.SIDToDomainCache.TryAdd(name, sid); + + if (LooksLikeDnsDomainName(name)) + { + CacheInstance.SIDToDomainCache.TryAdd(sid, name); + } + } + + private static bool LooksLikeDomainSid(string value) + { + return value != null && value.StartsWith("S-1-", StringComparison.OrdinalIgnoreCase); + } + + private static bool LooksLikeDnsDomainName(string value) + { + return value != null && value.IndexOf('.') >= 0; } /// diff --git a/src/CommonLib/ConnectionPoolManager.cs b/src/CommonLib/ConnectionPoolManager.cs index 19d92909b..9de2cca26 100644 --- a/src/CommonLib/ConnectionPoolManager.cs +++ b/src/CommonLib/ConnectionPoolManager.cs @@ -146,6 +146,13 @@ private string ResolveIdentifier(string identifier) { .GetAwaiter().GetResult(); if (infoOk && !string.IsNullOrEmpty(info?.DomainSid)) { Cache.AddDomainSidMapping(domainName, info.DomainSid); + // Also seed the canonical FQDN keyed write so the SID->Name slot is populated + // even when the caller passed a NetBIOS alias. AddDomainSidMapping gates the + // SID->Name direction on the name being DNS-shaped. + if (!string.IsNullOrEmpty(info.Name) && + !string.Equals(domainName, info.Name, StringComparison.OrdinalIgnoreCase)) { + Cache.AddDomainSidMapping(info.Name, info.DomainSid); + } return (true, info.DomainSid); } diff --git a/src/CommonLib/LdapUtils.cs b/src/CommonLib/LdapUtils.cs index d09d61ba7..98c137c03 100644 --- a/src/CommonLib/LdapUtils.cs +++ b/src/CommonLib/LdapUtils.cs @@ -456,6 +456,14 @@ public IAsyncEnumerable> PagedQuery(LdapQueryParame if (await GetDomainInfoAsync(domainName) is (true, var domainInfo) && !string.IsNullOrEmpty(domainInfo?.DomainSid)) { Cache.AddDomainSidMapping(domainName, domainInfo.DomainSid); + // Also seed the canonical FQDN keyed write so the SID->Name slot is populated + // even when the caller passed a NetBIOS alias. AddDomainSidMapping gates the + // SID->Name direction on the name being DNS-shaped, so the NetBIOS-keyed call + // above only writes Name->SID; this second call fills in the FQDN side. + if (!string.IsNullOrEmpty(domainInfo.Name) && + !string.Equals(domainName, domainInfo.Name, StringComparison.OrdinalIgnoreCase)) { + Cache.AddDomainSidMapping(domainInfo.Name, domainInfo.DomainSid); + } return (true, domainInfo.DomainSid); } diff --git a/test/unit/CacheTest.cs b/test/unit/CacheTest.cs index 97814ada2..3ea78c9df 100644 --- a/test/unit/CacheTest.cs +++ b/test/unit/CacheTest.cs @@ -128,6 +128,71 @@ public void AddDomainSidMapping_NullOrEmptyArgumentsAreIgnored(string key, strin Cache.SetCacheInstance(null); } + [Fact] + public void AddDomainSidMapping_NetBiosName_DoesNotPoisonSidToNameSlot() + { + // A NetBIOS-only write must not populate the SID->Name slot, because consumers of + // that slot expect a DNS-shaped FQDN (LDAP base DN construction, server selection, + // GetDomainInfoAsync hint resolution). Leaving the slot empty lets a later + // FQDN-keyed write populate it correctly. + Cache.SetCacheInstance(Cache.CreateNewCache()); + + const string sid = "S-1-5-21-1212121212-3434343434-5656565656"; + const string netbios = "CONTOSO"; + + Cache.AddDomainSidMapping(sid, netbios); + + Assert.False(Cache.GetDomainSidMapping(sid, out _)); + Assert.True(Cache.GetDomainSidMapping(netbios, out var resolvedSid)); + Assert.Equal(sid, resolvedSid); + + Cache.SetCacheInstance(null); + } + + [Fact] + public void AddDomainSidMapping_NetBiosName_ReverseArgumentOrder_DoesNotPoisonSidToNameSlot() + { + // Same poisoning vector with arguments reversed: AddDomainSidMapping(netbios, sid). + Cache.SetCacheInstance(Cache.CreateNewCache()); + + const string sid = "S-1-5-21-7878787878-9090909090-1212121212"; + const string netbios = "FABRIKAM"; + + Cache.AddDomainSidMapping(netbios, sid); + + Assert.False(Cache.GetDomainSidMapping(sid, out _)); + Assert.True(Cache.GetDomainSidMapping(netbios, out var resolvedSid)); + Assert.Equal(sid, resolvedSid); + + Cache.SetCacheInstance(null); + } + + [Fact] + public void AddDomainSidMapping_NetBiosThenFqdn_FqdnPopulatesSidToNameSlot() + { + // After a NetBIOS-keyed write leaves the SID->Name slot empty, a subsequent + // FQDN-keyed write must fill it with the canonical DNS name. This is the key + // behavior that makes the gating safe: the slot stays available for the richer + // resolver to populate. + Cache.SetCacheInstance(Cache.CreateNewCache()); + + const string sid = "S-1-5-21-3434343434-5656565656-7878787878"; + const string netbios = "CONTOSO"; + const string fqdn = "CONTOSO.LOCAL"; + + Cache.AddDomainSidMapping(netbios, sid); + Cache.AddDomainSidMapping(fqdn, sid); + + Assert.True(Cache.GetDomainSidMapping(sid, out var resolvedName)); + Assert.Equal(fqdn, resolvedName); + Assert.True(Cache.GetDomainSidMapping(netbios, out var resolvedFromNetbios)); + Assert.Equal(sid, resolvedFromNetbios); + Assert.True(Cache.GetDomainSidMapping(fqdn, out var resolvedFromFqdn)); + Assert.Equal(sid, resolvedFromFqdn); + + Cache.SetCacheInstance(null); + } + [Fact] public void SetCacheInstance_AfterDeserialization_RestoresCaseInsensitiveSidToDomain() { From 37004c82f256e58cc80f49f7d81e0514d7621d37 Mon Sep 17 00:00:00 2001 From: rvazarkar Date: Tue, 28 Apr 2026 16:20:22 -0400 Subject: [PATCH 13/20] chore: remove domain hint cache as we rarely call this function now and other caching will take care of it --- src/CommonLib/LdapUtils.cs | 62 +++++++++++--------------------------- 1 file changed, 18 insertions(+), 44 deletions(-) diff --git a/src/CommonLib/LdapUtils.cs b/src/CommonLib/LdapUtils.cs index 98c137c03..856b19977 100644 --- a/src/CommonLib/LdapUtils.cs +++ b/src/CommonLib/LdapUtils.cs @@ -35,13 +35,6 @@ public class LdapUtils : ILdapUtils { private static ConcurrentHashSet _domainControllers = new(StringComparer.OrdinalIgnoreCase); private static ConcurrentHashSet _unresolvablePrincipals = new(StringComparer.OrdinalIgnoreCase); - // Single-shot cache for the uncontrolled Domain.GetDomain() hint tier in - // ResolveEffectiveDomainHint. null = not attempted; "" = attempted and failed; otherwise the - // resolved domain name. Guarded by _uncontrolledGetDomainHintLock because the underlying RPC - // is slow and we only want it to fire once per process. Reset from ResetUtils. - private static string _uncontrolledGetDomainHint; - private static readonly object _uncontrolledGetDomainHintLock = new(); - private static readonly ConcurrentDictionary SeenWellKnownPrincipals = new(); @@ -1409,7 +1402,9 @@ internal static DomainInfo SelectRicherDomainInfo(DomainInfo seed, DomainInfo en /// target, letting the SDS/DsGetDcName stack resolve the current user's domain from the /// thread's outbound authentication context. In runas /netonly that context is the /// alt credential (not the local primary token that step 3 failed on), so this recovers - /// the real target domain. Result cached process-wide after the first successful attempt. + /// the real target domain. The RPC is issued on every call that reaches this tier; the + /// resolved hint becomes the key on success so subsequent + /// resolutions for the same domain short-circuit at the controlled-LDAP tier. /// Last-resort even when it equals the /// machine name. Downstream tiers will almost certainly fail to bind against this, but /// returning the env var here keeps behavior identical to the pre-change code for users who @@ -1466,12 +1461,6 @@ internal string ResolveEffectiveDomainHint(string domainName) { /// supplying it to could misdirect the locator; the /// credential-less form instead lets Windows use whatever authentication context is already /// bound to the thread. - /// - /// Result is cached process-wide in under - /// . The lock serializes the RPC so it fires at - /// most once per cycle even under concurrent callers. A stored empty - /// string is a "tried and failed" sentinel so repeat callers short-circuit without retrying. - /// /// private static bool TryResolveHintViaUncontrolledGetDomain( LdapConfig config, ILogger log, out string domainName) { @@ -1479,7 +1468,7 @@ private static bool TryResolveHintViaUncontrolledGetDomain( // Hard gate: uncontrolled calls require explicit opt-in. if (config is not { AllowFallbackToUncontrolledLdap: true }) return false; - + if (!string.IsNullOrWhiteSpace(config.Server)) { log.LogDebug( "TryResolveHintViaUncontrolledGetDomain(\"{Name}\", out string DomainName) short-circuited: Specific Server is set", @@ -1487,35 +1476,23 @@ private static bool TryResolveHintViaUncontrolledGetDomain( return false; } - lock (_uncontrolledGetDomainHintLock) { - // Previously resolved or previously failed - either way, no new RPC. - if (_uncontrolledGetDomainHint != null) { - if (_uncontrolledGetDomainHint.Length == 0) return false; - domainName = _uncontrolledGetDomainHint; + try { + // No name, no credentials: SDS resolves via the thread's outbound auth context, + // which is what surfaces the alt-creds domain under runas /netonly. + var ctx = new DirectoryContext(DirectoryContextType.Domain); + var domain = Domain.GetDomain(ctx); + var name = domain?.Name; + if (!string.IsNullOrEmpty(name)) { + domainName = name; return true; } - - try { - // No name, no credentials: SDS resolves via the thread's outbound auth context, - // which is what surfaces the alt-creds domain under runas /netonly. - var ctx = new DirectoryContext(DirectoryContextType.Domain); - var domain = Domain.GetDomain(ctx); - var name = domain?.Name; - if (!string.IsNullOrEmpty(name)) { - _uncontrolledGetDomainHint = name; - domainName = name; - return true; - } - } - catch (Exception e) { - log?.LogDebug(e, - "TryResolveHintViaUncontrolledGetDomain: Domain.GetDomain failed"); - } - - // Negative-cache the failure so repeat cache-miss paths don't re-issue the RPC. - _uncontrolledGetDomainHint = string.Empty; - return false; } + catch (Exception e) { + log?.LogDebug(e, + "TryResolveHintViaUncontrolledGetDomain: Domain.GetDomain failed"); + } + + return false; } /// @@ -2289,9 +2266,6 @@ public void ResetUtils() { lock (_currentDomainLock) { _currentDomain = null; } - lock (_uncontrolledGetDomainHintLock) { - _uncontrolledGetDomainHint = null; - } _connectionPool?.Dispose(); _connectionPool = new ConnectionPoolManager(_ldapConfig, scanner: _portScanner); From 7863689a2671dd2615447bf6add823866a596805 Mon Sep 17 00:00:00 2001 From: rvazarkar Date: Tue, 28 Apr 2026 17:49:48 -0400 Subject: [PATCH 14/20] fix: properly reset ExcludedDomains when resetting utils fix: invalid cached path fix: DCInfoCache case sensitivity fix: a race condition for domain info resolution that could result in multiple callers attempting to resolve the same domain, use lazy loading instead --- src/CommonLib/LdapConnectionPool.cs | 10 ++- src/CommonLib/LdapQueryParameters.cs | 71 +++++++++++----------- src/CommonLib/LdapUtils.cs | 91 +++++++++++++++++++++++++--- 3 files changed, 126 insertions(+), 46 deletions(-) diff --git a/src/CommonLib/LdapConnectionPool.cs b/src/CommonLib/LdapConnectionPool.cs index 6b9337092..b12ffe956 100644 --- a/src/CommonLib/LdapConnectionPool.cs +++ b/src/CommonLib/LdapConnectionPool.cs @@ -36,14 +36,18 @@ internal class LdapConnectionPool : IDisposable { private static readonly TimeSpan MaxBackoffDelay = TimeSpan.FromSeconds(20); private const int BackoffDelayMultiplier = 2; private const int MaxRetries = 3; - private static readonly ConcurrentDictionary DCInfoCache = new(); + private static readonly ConcurrentDictionary DCInfoCache = new(StringComparer.OrdinalIgnoreCase); // Metrics private readonly IMetricRouter _metric; - // Tracks domains we know we've determined we shouldn't try to connect to + // Tracks domains we know we've determined we shouldn't try to connect to. private static readonly ConcurrentHashSet ExcludedDomains = new(StringComparer.OrdinalIgnoreCase); + // Drops every exclusion record. Called from LdapUtils.ResetUtils so a fresh enumeration + // pass after a configuration change isn't shadowed by stale exclusion state. + internal static void ClearExclusions() => ExcludedDomains.Clear(); + public LdapConnectionPool(string identifier, string poolIdentifier, LdapConfig config, IPortScanner scanner = null, NativeMethods nativeMethods = null, ILogger log = null, IMetricRouter metric = null) { _connections = []; @@ -693,7 +697,7 @@ private static TimeSpan GetNextBackoff(int retryCount) { string tempPath; if (CallDsGetDcName(queryParameters.DomainName, out var info) && info != null) { tempPath = Helpers.DomainNameToDistinguishedName(info.Value.DomainName); - connectionWrapper.SaveContext(queryParameters.NamingContext, basePath); + connectionWrapper.SaveContext(queryParameters.NamingContext, tempPath); } else { // Controlled replacement for LdapUtils.GetDomain + DomainNameToDistinguishedName. diff --git a/src/CommonLib/LdapQueryParameters.cs b/src/CommonLib/LdapQueryParameters.cs index 354bf730f..2f2f09f63 100644 --- a/src/CommonLib/LdapQueryParameters.cs +++ b/src/CommonLib/LdapQueryParameters.cs @@ -3,46 +3,47 @@ using System.Threading; using SharpHoundCommonLib.Enums; -namespace SharpHoundCommonLib { - public class LdapQueryParameters { - private static int _queryIDIndex; - private string _searchBase; - private string _relativeSearchBase; - public string LDAPFilter { get; set; } - public SearchScope SearchScope { get; set; } = SearchScope.Subtree; - public string[] Attributes { get; set; } = Array.Empty(); - public string DomainName { get; set; } - public bool GlobalCatalog { get; set; } - public bool IncludeSecurityDescriptor { get; set; } = false; - public bool IncludeDeleted { get; set; } = false; - private int QueryID { get; } - - public LdapQueryParameters() { - QueryID = _queryIDIndex; - Interlocked.Increment(ref _queryIDIndex); - } +namespace SharpHoundCommonLib; + +public class LdapQueryParameters { + private static int _queryIDIndex; + private string _relativeSearchBase; + private string _searchBase; + + public LdapQueryParameters() { + QueryID = _queryIDIndex; + Interlocked.Increment(ref _queryIDIndex); + } - public string SearchBase { - get => _searchBase; - set { - _relativeSearchBase = null; - _searchBase = value; - } + public string LDAPFilter { get; set; } + public SearchScope SearchScope { get; set; } = SearchScope.Subtree; + public string[] Attributes { get; set; } = Array.Empty(); + public string DomainName { get; set; } + public bool GlobalCatalog { get; set; } + public bool IncludeSecurityDescriptor { get; set; } = false; + public bool IncludeDeleted { get; set; } = false; + private int QueryID { get; } + + public string SearchBase { + get => _searchBase; + set { + _relativeSearchBase = null; + _searchBase = value; } + } - public string RelativeSearchBase { - get => _relativeSearchBase; - set { - _relativeSearchBase = value; - _searchBase = null; - } + public string RelativeSearchBase { + get => _relativeSearchBase; + set { + _relativeSearchBase = value; + _searchBase = null; } + } - public NamingContext NamingContext { get; set; } = NamingContext.Default; + public NamingContext NamingContext { get; set; } = NamingContext.Default; - public string GetQueryInfo() - { - return $"Query Information - Filter: {LDAPFilter}, Domain: {DomainName}, GlobalCatalog: {GlobalCatalog}, ADSPath: {SearchBase}, ID: {QueryID}"; - } + public string GetQueryInfo() { + return + $"Query Information - Filter: {LDAPFilter}, Domain: {DomainName}, GlobalCatalog: {GlobalCatalog}, ADSPath: {SearchBase}, ID: {QueryID}"; } } \ No newline at end of file diff --git a/src/CommonLib/LdapUtils.cs b/src/CommonLib/LdapUtils.cs index 856b19977..ad0cb0916 100644 --- a/src/CommonLib/LdapUtils.cs +++ b/src/CommonLib/LdapUtils.cs @@ -35,6 +35,22 @@ public class LdapUtils : ILdapUtils { private static ConcurrentHashSet _domainControllers = new(StringComparer.OrdinalIgnoreCase); private static ConcurrentHashSet _unresolvablePrincipals = new(StringComparer.OrdinalIgnoreCase); + // Coalesces concurrent first-time domain resolutions so that N callers asking for the + // same domain trigger one tier walk, not N. The first caller's pool/config/log is captured + // by the lazy; subsequent callers await the same task and see the same result. Entries + // are removed once the task settles, so a later cache miss for the same domain (e.g., + // post-ResetUtils) starts a fresh resolution rather than reusing a completed task. + private static readonly ConcurrentDictionary>> + _inFlightDomainResolutions = new(StringComparer.OrdinalIgnoreCase); + + // Tracks which domains are currently being resolved on the calling logical execution + // context. Reentrant resolution is by design: the pool tier's GetLdapConnection path + // calls back into GetDomainInfoStaticAsync to resolve the domain SID before binding. + // Without this guard the inner call would await the outer lazy that hasn't published + // yet, deadlocking on Lazy's self-recursion detection. AsyncLocal flows through async + // continuations, so it captures the recursion chain across any number of awaits. + private static readonly AsyncLocal> _resolvingDomains = new(); + private static readonly ConcurrentDictionary SeenWellKnownPrincipals = new(); @@ -1321,12 +1337,71 @@ internal static DomainInfo SelectRicherDomainInfo(DomainInfo seed, DomainInfo en } /// - /// Shared resolution core that drives the four-tier walk for both + /// Shared resolution entry point that drives the four-tier walk for both /// and . /// Tier 1 (pool-driven controlled LDAP) is skipped when is null. /// Tiers 2-4 (one-shot direct LDAP, ADSI, uncontrolled fallback) run unconditionally. /// /// + /// Concurrent first-time resolutions for the same domain are coalesced through + /// so duplicate callers observe the same + /// richest-tier-published record instead of each issuing the full tier walk independently. + /// The first caller's , , and + /// are captured by the lazy; subsequent awaiters inherit those. + /// In practice all instances in a process share the same effective + /// config, so this matches the ambient assumption already made by the static + /// . + /// + /// A pool-equipped caller treats a cached record produced by a non-pool tier + /// (signaled by an absent , since only the pool tier + /// reads CN=Partitions) as still re-resolvable. Otherwise an earlier sparse write + /// from a re-entrant call would permanently shadow + /// the richer record this caller's pool could produce. + /// + /// + private static async Task<(bool Success, DomainInfo DomainInfo)> ResolveDomainInfoAsync( + string domainName, ConnectionPoolManager pool, LdapConfig config, ILogger log) { + if (string.IsNullOrWhiteSpace(domainName)) { + return (false, null); + } + + if (_domainInfoCache.TryGetValue(domainName, out var cached) + && (pool == null || !string.IsNullOrEmpty(cached.NetBiosName))) { + return (true, cached); + } + + // Reentrant resolution on the same async chain (pool tier -> GetDomainSidFromDomainName -> + // GetDomainInfoStaticAsync -> back here) bypasses the lazy: the outer lazy hasn't published + // a Task yet, so awaiting it would self-deadlock. The recursion is bounded because the + // inner call always runs with pool=null and so cannot re-enter the pool tier. + var resolving = _resolvingDomains.Value ??= new HashSet(StringComparer.OrdinalIgnoreCase); + if (!resolving.Add(domainName)) { + return await ResolveDomainInfoCoreAsync(domainName, pool, config, log); + } + + try { + var lazy = _inFlightDomainResolutions.GetOrAdd(domainName, + key => new Lazy>( + () => ResolveDomainInfoCoreAsync(key, pool, config, log), + LazyThreadSafetyMode.ExecutionAndPublication)); + + try { + return await lazy.Value.ConfigureAwait(false); + } + finally { + _inFlightDomainResolutions.TryRemove(domainName, out _); + } + } + finally { + resolving.Remove(domainName); + } + } + + /// + /// Drives the actual four-tier walk for . Wrapped by a + /// per-domain so concurrent first-time callers share the result. + /// + /// /// Tier ordering rationale: the pool tier honors every flag and /// is preferred when available. The one-shot direct-LDAP tier is tried before ADSI because /// it is the only tier outside the pool that can express fine-grained @@ -1336,13 +1411,12 @@ internal static DomainInfo SelectRicherDomainInfo(DomainInfo seed, DomainInfo en /// direct-LDAP enrichment pass against the discovered PDC so the cached record reaches the /// same shape as the pool and one-shot tiers. /// - private static async Task<(bool Success, DomainInfo DomainInfo)> ResolveDomainInfoAsync( + private static async Task<(bool Success, DomainInfo DomainInfo)> ResolveDomainInfoCoreAsync( string domainName, ConnectionPoolManager pool, LdapConfig config, ILogger log) { - if (string.IsNullOrWhiteSpace(domainName)) { - return (false, null); - } - - if (_domainInfoCache.TryGetValue(domainName, out var cached)) { + // Re-check inside the lazy: another caller may have published a satisfying record + // between our outer cache miss and this lazy's first execution. + if (_domainInfoCache.TryGetValue(domainName, out var cached) + && (pool == null || !string.IsNullOrEmpty(cached.NetBiosName))) { return (true, cached); } @@ -1470,7 +1544,7 @@ private static bool TryResolveHintViaUncontrolledGetDomain( if (config is not { AllowFallbackToUncontrolledLdap: true }) return false; if (!string.IsNullOrWhiteSpace(config.Server)) { - log.LogDebug( + log?.LogDebug( "TryResolveHintViaUncontrolledGetDomain(\"{Name}\", out string DomainName) short-circuited: Specific Server is set", domainName); return false; @@ -2266,6 +2340,7 @@ public void ResetUtils() { lock (_currentDomainLock) { _currentDomain = null; } + LdapConnectionPool.ClearExclusions(); _connectionPool?.Dispose(); _connectionPool = new ConnectionPoolManager(_ldapConfig, scanner: _portScanner); From 6d2a1a43fd28642f0d1b0190ff921a9b497624f7 Mon Sep 17 00:00:00 2001 From: rvazarkar Date: Tue, 28 Apr 2026 18:15:46 -0400 Subject: [PATCH 15/20] fix: a potential crash in CopyCaseInsensitive fix: reset currentDomain when changing LDAPConfig --- src/CommonLib/Cache.cs | 26 ++++++++++++---- src/CommonLib/LdapUtils.cs | 6 ++++ test/unit/CacheTest.cs | 63 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 89 insertions(+), 6 deletions(-) diff --git a/src/CommonLib/Cache.cs b/src/CommonLib/Cache.cs index e1f43d456..f94a9bfad 100644 --- a/src/CommonLib/Cache.cs +++ b/src/CommonLib/Cache.cs @@ -204,21 +204,35 @@ private static void NormalizeCaseInsensitiveCaches() if (CacheInstance == null) return; if (CacheInstance.SIDToDomainCache != null) { - CacheInstance.SIDToDomainCache = new ConcurrentDictionary( - CacheInstance.SIDToDomainCache, StringComparer.OrdinalIgnoreCase); + CacheInstance.SIDToDomainCache = CopyCaseInsensitive(CacheInstance.SIDToDomainCache); } if (CacheInstance.GlobalCatalogCache != null) { - CacheInstance.GlobalCatalogCache = new ConcurrentDictionary( - CacheInstance.GlobalCatalogCache, StringComparer.OrdinalIgnoreCase); + CacheInstance.GlobalCatalogCache = CopyCaseInsensitive(CacheInstance.GlobalCatalogCache); } if (CacheInstance.ValueToIdCache != null) { - CacheInstance.ValueToIdCache = new ConcurrentDictionary( - CacheInstance.ValueToIdCache, StringComparer.OrdinalIgnoreCase); + CacheInstance.ValueToIdCache = CopyCaseInsensitive(CacheInstance.ValueToIdCache); } } + /// + /// Copies into a new keyed + /// by . Entries are added with TryAdd so keys that + /// collide only by case (introduced before the case-insensitive invariant was reapplied) are + /// silently dropped — first writer wins — rather than throwing from the constructor. + /// + private static ConcurrentDictionary CopyCaseInsensitive( + ConcurrentDictionary source) + { + var copy = new ConcurrentDictionary(StringComparer.OrdinalIgnoreCase); + foreach (var kvp in source) + { + copy.TryAdd(kvp.Key, kvp.Value); + } + return copy; + } + /// /// Gets stats from the currently loaded cache /// diff --git a/src/CommonLib/LdapUtils.cs b/src/CommonLib/LdapUtils.cs index ad0cb0916..1038fce9f 100644 --- a/src/CommonLib/LdapUtils.cs +++ b/src/CommonLib/LdapUtils.cs @@ -1136,6 +1136,12 @@ await GetDomainSidFromDomainName(forestName) is (true, var forestDomainSid)) { public void SetLdapConfig(LdapConfig config) { _ldapConfig = config; _log.LogInformation("New LDAP Config Set:\n {ConfigString}", config.ToString()); + // _currentDomain was resolved under the previous credentials/server, both of which + // can have just changed. Drop it so the next GetDomain(out _) re-resolves against the + // new auth context instead of returning a stale Domain bound to the old config. + lock (_currentDomainLock) { + _currentDomain = null; + } _connectionPool.Dispose(); _connectionPool = new ConnectionPoolManager(_ldapConfig, scanner: _portScanner); } diff --git a/test/unit/CacheTest.cs b/test/unit/CacheTest.cs index 3ea78c9df..324cd4d8f 100644 --- a/test/unit/CacheTest.cs +++ b/test/unit/CacheTest.cs @@ -295,5 +295,68 @@ public void SetCacheInstance_AfterDeserialization_RestoresCaseInsensitiveValueTo Cache.SetCacheInstance(null); } + + [Fact] + public void SetCacheInstance_AfterDeserialization_HealsCaseCollidingKeys() + { + // A persisted cache produced before the case-insensitive invariant was applied + // (or hand-edited / corrupted on disk) can contain two keys that differ only by + // case in the same dictionary. Deserialization rebuilds a case-sensitive + // ConcurrentDictionary, so both keys survive the round trip. The rewrap must + // tolerate the collision (first writer wins) instead of throwing from the + // ConcurrentDictionary(IEnumerable, IEqualityComparer) constructor. + // Each dictionary contains a pair of keys differing only by case. In a + // case-sensitive ConcurrentDictionary (what the deserializer rebuilds) both keys + // survive; rewrapping with OrdinalIgnoreCase against that source is the path that + // previously threw ArgumentException. + const string json = @"{ + ""SIDToDomainCache"": { + ""CONTOSO.LOCAL"": ""S-1-5-21-1-2-3"", + ""contoso.local"": ""S-1-5-21-1-2-3"" + }, + ""GlobalCatalogCache"": { + ""CONTOSO.LOCAL"": [""gc1.contoso.local""], + ""contoso.local"": [""gc1.contoso.local""] + }, + ""ValueToIdCache"": { + ""Administrator|CONTOSO.LOCAL"": ""S-1-5-21-1-2-3-500"", + ""administrator|contoso.local"": ""S-1-5-21-1-2-3-500"" + }, + ""IdToTypeCache"": {}, + ""MachineSidCache"": {}, + ""CacheCreationDate"": ""0001-01-01T00:00:00"", + ""CacheCreationVersion"": ""1.0.0"" + }"; + var settings = new JsonSerializerSettings + { + ObjectCreationHandling = ObjectCreationHandling.Replace + }; + var deserialized = JsonConvert.DeserializeObject(json, settings); + + // Sanity: the deserialized dictionaries actually contain the colliding keys. + // Without this, the test would not exercise the constructor's duplicate-key path. + Assert.Equal(2, deserialized.SIDToDomainCache.Count); + Assert.Equal(2, deserialized.GlobalCatalogCache.Count); + Assert.Equal(2, deserialized.ValueToIdCache.Count); + + // Pre-fix this call threw ArgumentException from the rewrap constructor. + var ex = Record.Exception(() => Cache.SetCacheInstance(deserialized)); + Assert.Null(ex); + + // Each cache collapses to a single entry per case-insensitive key (first wins). + Assert.Single(deserialized.SIDToDomainCache); + Assert.Single(deserialized.GlobalCatalogCache); + Assert.Single(deserialized.ValueToIdCache); + + // Lookups succeed under either casing after the rewrap. + Assert.True(Cache.GetDomainSidMapping("contoso.local", out var resolvedSid)); + Assert.Equal("S-1-5-21-1-2-3", resolvedSid); + Assert.True(Cache.GetGCCache("contoso.local", out var gcs)); + Assert.Single(gcs); + Assert.True(Cache.GetPrefixedValue("administrator", "contoso.local", out var resolvedId)); + Assert.Equal("S-1-5-21-1-2-3-500", resolvedId); + + Cache.SetCacheInstance(null); + } } } From f543cebfdc031ea3ac81b833cfd8d0c5b80b84b5 Mon Sep 17 00:00:00 2001 From: rvazarkar Date: Wed, 29 Apr 2026 01:05:44 -0400 Subject: [PATCH 16/20] chore: simplify lazy logic significantly, exclude static callers from the logic fix: reset DCInfoCache when utils are reset --- src/CommonLib/LdapConnectionPool.cs | 5 +- src/CommonLib/LdapUtils.cs | 98 ++++++++++++++--------------- 2 files changed, 50 insertions(+), 53 deletions(-) diff --git a/src/CommonLib/LdapConnectionPool.cs b/src/CommonLib/LdapConnectionPool.cs index b12ffe956..47e804cb0 100644 --- a/src/CommonLib/LdapConnectionPool.cs +++ b/src/CommonLib/LdapConnectionPool.cs @@ -46,7 +46,10 @@ internal class LdapConnectionPool : IDisposable { // Drops every exclusion record. Called from LdapUtils.ResetUtils so a fresh enumeration // pass after a configuration change isn't shadowed by stale exclusion state. - internal static void ClearExclusions() => ExcludedDomains.Clear(); + internal static void ResetCaches() { + DCInfoCache.Clear(); + ExcludedDomains.Clear(); + } public LdapConnectionPool(string identifier, string poolIdentifier, LdapConfig config, IPortScanner scanner = null, NativeMethods nativeMethods = null, ILogger log = null, IMetricRouter metric = null) { diff --git a/src/CommonLib/LdapUtils.cs b/src/CommonLib/LdapUtils.cs index 1038fce9f..daefeb7a8 100644 --- a/src/CommonLib/LdapUtils.cs +++ b/src/CommonLib/LdapUtils.cs @@ -35,22 +35,16 @@ public class LdapUtils : ILdapUtils { private static ConcurrentHashSet _domainControllers = new(StringComparer.OrdinalIgnoreCase); private static ConcurrentHashSet _unresolvablePrincipals = new(StringComparer.OrdinalIgnoreCase); - // Coalesces concurrent first-time domain resolutions so that N callers asking for the - // same domain trigger one tier walk, not N. The first caller's pool/config/log is captured - // by the lazy; subsequent callers await the same task and see the same result. Entries - // are removed once the task settles, so a later cache miss for the same domain (e.g., - // post-ResetUtils) starts a fresh resolution rather than reusing a completed task. + // Coalesces concurrent first-time domain resolutions issued through the instance + // GetDomainInfoAsync path so N callers asking for the same domain trigger one pool-driven + // tier walk instead of N. Only the coalesced (pool-equipped) path inserts here; the + // direct GetDomainInfoStaticAsync entry bypasses this dictionary entirely so the pool + // tier can re-enter for SID/DN resolution without self-deadlocking on the outer Lazy. + // Entries are removed once the task settles, so a later cache miss for the same domain + // (e.g., post-ResetUtils) starts a fresh resolution rather than reusing a completed task. private static readonly ConcurrentDictionary>> _inFlightDomainResolutions = new(StringComparer.OrdinalIgnoreCase); - // Tracks which domains are currently being resolved on the calling logical execution - // context. Reentrant resolution is by design: the pool tier's GetLdapConnection path - // calls back into GetDomainInfoStaticAsync to resolve the domain SID before binding. - // Without this guard the inner call would await the outer lazy that hasn't published - // yet, deadlocking on Lazy's self-recursion detection. AsyncLocal flows through async - // continuations, so it captures the recursion chain across any number of awaits. - private static readonly AsyncLocal> _resolvingDomains = new(); - private static readonly ConcurrentDictionary SeenWellKnownPrincipals = new(); @@ -1322,8 +1316,8 @@ internal static DomainInfo SelectRicherDomainInfo(DomainInfo seed, DomainInfo en } /// - /// Static counterpart to intended for internal - /// consumers that cannot hold an instance (notably + /// Static, uncoalesced counterpart to intended for + /// internal consumers that cannot hold an instance (notably /// and , which are /// themselves pieces of the connection infrastructure and can't reenter it transparently). /// @@ -1336,33 +1330,47 @@ internal static DomainInfo SelectRicherDomainInfo(DomainInfo seed, DomainInfo en /// calls on the same process. The pool tier is /// skipped here because no is available; the remaining /// three tiers (one-shot direct LDAP, ADSI, uncontrolled fallback) still run. + /// + /// Deliberately bypasses . This entry point exists + /// for re-entrant pool callers (the pool tier resolves a domain SID by calling here while + /// itself executing inside a coalesced instance walk for the same domain). Coalescing the + /// re-entrant call against the outer pool-tier Lazy would self-deadlock, since that Lazy + /// has not yet published its Task. Concurrent non-recursive callers will each run the + /// non-pool tier walk independently; the cache write performed by the first to finish + /// short-circuits subsequent callers via the cache check above. + /// /// internal static Task<(bool Success, DomainInfo DomainInfo)> GetDomainInfoStaticAsync( string domainName, LdapConfig config, ILogger log = null) { - return ResolveDomainInfoAsync(domainName, pool: null, config, log); + if (string.IsNullOrWhiteSpace(domainName)) { + return Task.FromResult<(bool, DomainInfo)>((false, null)); + } + + if (_domainInfoCache.TryGetValue(domainName, out var cached)) { + return Task.FromResult((true, cached)); + } + + return ResolveDomainInfoCoreAsync(domainName, pool: null, config, log); } /// - /// Shared resolution entry point that drives the four-tier walk for both - /// and . - /// Tier 1 (pool-driven controlled LDAP) is skipped when is null. - /// Tiers 2-4 (one-shot direct LDAP, ADSI, uncontrolled fallback) run unconditionally. + /// Coalesced resolution entry point used by the instance + /// path. Drives the full four-tier walk (pool LDAP, one-shot direct LDAP, ADSI, uncontrolled + /// fallback) and serializes concurrent first-time callers for the same domain through + /// so N callers observe one shared tier walk. /// /// - /// Concurrent first-time resolutions for the same domain are coalesced through - /// so duplicate callers observe the same - /// richest-tier-published record instead of each issuing the full tier walk independently. /// The first caller's , , and - /// are captured by the lazy; subsequent awaiters inherit those. - /// In practice all instances in a process share the same effective + /// are captured by the lazy; subsequent awaiters inherit those. In + /// practice all instances in a process share the same effective /// config, so this matches the ambient assumption already made by the static /// . /// - /// A pool-equipped caller treats a cached record produced by a non-pool tier - /// (signaled by an absent , since only the pool tier - /// reads CN=Partitions) as still re-resolvable. Otherwise an earlier sparse write - /// from a re-entrant call would permanently shadow - /// the richer record this caller's pool could produce. + /// Cached records produced by a non-pool tier (signaled by an absent + /// , since only the pool tier reads CN=Partitions) + /// are treated as still re-resolvable so a sparse seed written by an earlier + /// call doesn't permanently shadow the richer record + /// this caller's pool could produce. /// /// private static async Task<(bool Success, DomainInfo DomainInfo)> ResolveDomainInfoAsync( @@ -1372,34 +1380,20 @@ internal static DomainInfo SelectRicherDomainInfo(DomainInfo seed, DomainInfo en } if (_domainInfoCache.TryGetValue(domainName, out var cached) - && (pool == null || !string.IsNullOrEmpty(cached.NetBiosName))) { + && !string.IsNullOrEmpty(cached.NetBiosName)) { return (true, cached); } - // Reentrant resolution on the same async chain (pool tier -> GetDomainSidFromDomainName -> - // GetDomainInfoStaticAsync -> back here) bypasses the lazy: the outer lazy hasn't published - // a Task yet, so awaiting it would self-deadlock. The recursion is bounded because the - // inner call always runs with pool=null and so cannot re-enter the pool tier. - var resolving = _resolvingDomains.Value ??= new HashSet(StringComparer.OrdinalIgnoreCase); - if (!resolving.Add(domainName)) { - return await ResolveDomainInfoCoreAsync(domainName, pool, config, log); - } + var lazy = _inFlightDomainResolutions.GetOrAdd(domainName, + key => new Lazy>( + () => ResolveDomainInfoCoreAsync(key, pool, config, log), + LazyThreadSafetyMode.ExecutionAndPublication)); try { - var lazy = _inFlightDomainResolutions.GetOrAdd(domainName, - key => new Lazy>( - () => ResolveDomainInfoCoreAsync(key, pool, config, log), - LazyThreadSafetyMode.ExecutionAndPublication)); - - try { - return await lazy.Value.ConfigureAwait(false); - } - finally { - _inFlightDomainResolutions.TryRemove(domainName, out _); - } + return await lazy.Value.ConfigureAwait(false); } finally { - resolving.Remove(domainName); + _inFlightDomainResolutions.TryRemove(domainName, out _); } } @@ -2346,7 +2340,7 @@ public void ResetUtils() { lock (_currentDomainLock) { _currentDomain = null; } - LdapConnectionPool.ClearExclusions(); + LdapConnectionPool.ResetCaches(); _connectionPool?.Dispose(); _connectionPool = new ConnectionPoolManager(_ldapConfig, scanner: _portScanner); From 04a18f20f32a76ce0f4c0e0f12b835e84736be96 Mon Sep 17 00:00:00 2001 From: rvazarkar Date: Wed, 29 Apr 2026 01:21:12 -0400 Subject: [PATCH 17/20] fix: significantly tighten up logic for checking if we're caching a DNS name to avoid cache poisoning --- src/CommonLib/Cache.cs | 49 ++++++++++++++++++++++++++++++- test/unit/CacheTest.cs | 66 ++++++++++++++++++++++++++++++++---------- 2 files changed, 98 insertions(+), 17 deletions(-) diff --git a/src/CommonLib/Cache.cs b/src/CommonLib/Cache.cs index f94a9bfad..089cc083e 100644 --- a/src/CommonLib/Cache.cs +++ b/src/CommonLib/Cache.cs @@ -86,9 +86,56 @@ private static bool LooksLikeDomainSid(string value) return value != null && value.StartsWith("S-1-", StringComparison.OrdinalIgnoreCase); } + // Gates the SID->Name reverse cache write. Downstream consumers (LDAP base DN + // construction, server selection, GetDomainInfoAsync hint resolution) treat the value in + // that slot as an FQDN they can split on '.' and feed to DC=/CN=Partitions queries, so + // a NetBIOS alias, an IPv4 literal, or a malformed label set in this slot poisons every + // downstream lookup for the SID. The check enforces RFC-1035-shaped multi-label DNS: + // total length <= 253, >= 2 non-empty labels, each label 1-63 chars of [A-Za-z0-9-] + // with no leading/trailing hyphen, and not an IPv4 literal (four all-digit labels). + // Single-label AD domains (e.g., a forest root literally named "CORP") are deliberately + // rejected: a single label is syntactically indistinguishable from a NetBIOS alias and + // we'd rather slow-path-resolve them than silently cache the wrong shape. private static bool LooksLikeDnsDomainName(string value) { - return value != null && value.IndexOf('.') >= 0; + if (string.IsNullOrEmpty(value) || value.Length > 253) return false; + + var labels = value.Split('.'); + if (labels.Length < 2) return false; + + var allNumeric = true; + foreach (var label in labels) + { + if (!IsValidDnsLabel(label)) return false; + if (allNumeric && !IsAllDigits(label)) allNumeric = false; + } + + // Reject IPv4 literals after per-label validation so we don't preempt a malformed-input + // rejection with a shape-based one (the diagnostic value is in the per-label check). + return !(labels.Length == 4 && allNumeric); + } + + private static bool IsValidDnsLabel(string label) + { + if (label.Length == 0 || label.Length > 63) return false; + if (label[0] == '-' || label[label.Length - 1] == '-') return false; + + foreach (var c in label) + { + var isDigit = c >= '0' && c <= '9'; + var isAlpha = (c >= 'A' && c <= 'Z') || (c >= 'a' && c <= 'z'); + if (!isDigit && !isAlpha && c != '-') return false; + } + return true; + } + + private static bool IsAllDigits(string s) + { + foreach (var c in s) + { + if (c < '0' || c > '9') return false; + } + return true; } /// diff --git a/test/unit/CacheTest.cs b/test/unit/CacheTest.cs index 324cd4d8f..1b10c7a47 100644 --- a/test/unit/CacheTest.cs +++ b/test/unit/CacheTest.cs @@ -129,18 +129,15 @@ public void AddDomainSidMapping_NullOrEmptyArgumentsAreIgnored(string key, strin } [Fact] - public void AddDomainSidMapping_NetBiosName_DoesNotPoisonSidToNameSlot() + public void AddDomainSidMapping_NetBiosName_ReverseArgumentOrder_DoesNotPoisonSidToNameSlot() { - // A NetBIOS-only write must not populate the SID->Name slot, because consumers of - // that slot expect a DNS-shaped FQDN (LDAP base DN construction, server selection, - // GetDomainInfoAsync hint resolution). Leaving the slot empty lets a later - // FQDN-keyed write populate it correctly. + // Same poisoning vector with arguments reversed: AddDomainSidMapping(netbios, sid). Cache.SetCacheInstance(Cache.CreateNewCache()); - const string sid = "S-1-5-21-1212121212-3434343434-5656565656"; - const string netbios = "CONTOSO"; + const string sid = "S-1-5-21-7878787878-9090909090-1212121212"; + const string netbios = "FABRIKAM"; - Cache.AddDomainSidMapping(sid, netbios); + Cache.AddDomainSidMapping(netbios, sid); Assert.False(Cache.GetDomainSidMapping(sid, out _)); Assert.True(Cache.GetDomainSidMapping(netbios, out var resolvedSid)); @@ -149,19 +146,56 @@ public void AddDomainSidMapping_NetBiosName_DoesNotPoisonSidToNameSlot() Cache.SetCacheInstance(null); } - [Fact] - public void AddDomainSidMapping_NetBiosName_ReverseArgumentOrder_DoesNotPoisonSidToNameSlot() + [Theory] + [InlineData("10.0.0.1")] // IPv4 literal + [InlineData("192.168.1.100")] // IPv4 literal + [InlineData("contoso.")] // trailing-dot artifact + [InlineData(".contoso.com")] // leading-dot junk + [InlineData("contoso..com")] // empty middle label + [InlineData("-contoso.com")] // leading hyphen + [InlineData("contoso-.com")] // trailing hyphen on label + [InlineData("contoso.com-")] // trailing hyphen on FQDN + [InlineData("contoso.local!")] // invalid character + [InlineData("contoso .local")] // embedded space + [InlineData("CORP")] // single-label (ambiguous with NetBIOS) + public void AddDomainSidMapping_NonDnsShapedName_DoesNotPoisonSidToNameSlot(string badName) { - // Same poisoning vector with arguments reversed: AddDomainSidMapping(netbios, sid). + // Anything that isn't unambiguously an RFC-1035 multi-label FQDN gets rejected from + // the SID->Name slot. IPv4 literals and malformed label sets are the legacy-environment + // vectors that the old IndexOf('.') heuristic let through. NetBIOS names that happen + // to contain a dot (e.g. "CONTOSO.OLD") are syntactically indistinguishable from real + // FQDNs and remain an unavoidable false-positive of any pure-syntax check. Cache.SetCacheInstance(Cache.CreateNewCache()); - const string sid = "S-1-5-21-7878787878-9090909090-1212121212"; - const string netbios = "FABRIKAM"; - - Cache.AddDomainSidMapping(netbios, sid); + const string sid = "S-1-5-21-1010101010-2020202020-3030303030"; + Cache.AddDomainSidMapping(sid, badName); Assert.False(Cache.GetDomainSidMapping(sid, out _)); - Assert.True(Cache.GetDomainSidMapping(netbios, out var resolvedSid)); + Assert.True(Cache.GetDomainSidMapping(badName, out var resolvedSid)); + Assert.Equal(sid, resolvedSid); + + Cache.SetCacheInstance(null); + } + + [Theory] + [InlineData("contoso.local")] // typical AD FQDN + [InlineData("CONTOSO.LOCAL")] // upper-case + [InlineData("sub.contoso.local")] // 3 labels + [InlineData("a.b.c.d.e")] // 5 labels (rules out 4-label IPv4 collision) + [InlineData("dc-01.contoso.local")] // mid-label hyphen + [InlineData("123abc.contoso.local")] // leading digit per RFC 1123 + public void AddDomainSidMapping_DnsShapedName_PopulatesSidToNameSlot(string fqdn) + { + // Positive coverage for the tightened heuristic: legitimate FQDNs still populate + // both directions of the cache. + Cache.SetCacheInstance(Cache.CreateNewCache()); + + const string sid = "S-1-5-21-4040404040-5050505050-6060606060"; + Cache.AddDomainSidMapping(sid, fqdn); + + Assert.True(Cache.GetDomainSidMapping(sid, out var resolvedName)); + Assert.Equal(fqdn, resolvedName); + Assert.True(Cache.GetDomainSidMapping(fqdn, out var resolvedSid)); Assert.Equal(sid, resolvedSid); Cache.SetCacheInstance(null); From 9062696a41ddf0cfaa0101bd86285d03a72280b4 Mon Sep 17 00:00:00 2001 From: rvazarkar Date: Wed, 29 Apr 2026 01:44:18 -0400 Subject: [PATCH 18/20] fix: move pool resolution above ADSI in GetDomainSidFromDomainName to match our expected precedence --- src/CommonLib/LdapUtils.cs | 51 +++++++++++++++++++------------------- 1 file changed, 26 insertions(+), 25 deletions(-) diff --git a/src/CommonLib/LdapUtils.cs b/src/CommonLib/LdapUtils.cs index daefeb7a8..8e5a37c65 100644 --- a/src/CommonLib/LdapUtils.cs +++ b/src/CommonLib/LdapUtils.cs @@ -440,20 +440,7 @@ public IAsyncEnumerable> PagedQuery(LdapQueryParame public virtual async Task<(bool Success, string DomainSid)> GetDomainSidFromDomainName(string domainName) { if (Cache.GetDomainSidMapping(domainName, out var domainSid)) return (true, domainSid); - - try { - var entry = Helpers.CreateDirectoryEntry($"LDAP://{domainName}", _ldapConfig); - //Force load objectsid into the object cache - if (entry.TryGetSecurityIdentifier(out var sid)) { - Cache.AddDomainSidMapping(domainName, sid); - domainSid = sid; - return (true, domainSid); - } - } - catch { - //we expect this to fail sometimes - } - + // Replaces the legacy GetDomain(out Domain) + GetDirectoryEntry block with a // controlled lookup via the connection pool. if (await GetDomainInfoAsync(domainName) is (true, var domainInfo) && @@ -470,6 +457,19 @@ public IAsyncEnumerable> PagedQuery(LdapQueryParame return (true, domainInfo.DomainSid); } + try { + var entry = Helpers.CreateDirectoryEntry($"LDAP://{domainName}", _ldapConfig); + //Force load objectsid into the object cache + if (entry.TryGetSecurityIdentifier(out var sid)) { + Cache.AddDomainSidMapping(domainName, sid); + domainSid = sid; + return (true, domainSid); + } + } + catch { + //we expect this to fail sometimes + } + foreach (var name in _translateNames) try { var account = new NTAccount(domainName, name); @@ -1134,6 +1134,7 @@ public void SetLdapConfig(LdapConfig config) { // can have just changed. Drop it so the next GetDomain(out _) re-resolves against the // new auth context instead of returning a stale Domain bound to the old config. lock (_currentDomainLock) { + _currentDomain?.Dispose(); _currentDomain = null; } _connectionPool.Dispose(); @@ -1366,11 +1367,12 @@ internal static DomainInfo SelectRicherDomainInfo(DomainInfo seed, DomainInfo en /// config, so this matches the ambient assumption already made by the static /// . /// - /// Cached records produced by a non-pool tier (signaled by an absent - /// , since only the pool tier reads CN=Partitions) - /// are treated as still re-resolvable so a sparse seed written by an earlier - /// call doesn't permanently shadow the richer record - /// this caller's pool could produce. + /// Any cached record satisfies this lookup regardless of which tier produced it. Upgrade + /// from a sparser seed to a richer record happens at write time via + /// in ; the read path does not + /// re-resolve a cached domain just because some attribute (e.g. ) + /// is absent, since attributes that are unreachable for the configured credentials would + /// otherwise trigger an unbounded re-resolution loop on every call. /// /// private static async Task<(bool Success, DomainInfo DomainInfo)> ResolveDomainInfoAsync( @@ -1379,8 +1381,7 @@ internal static DomainInfo SelectRicherDomainInfo(DomainInfo seed, DomainInfo en return (false, null); } - if (_domainInfoCache.TryGetValue(domainName, out var cached) - && !string.IsNullOrEmpty(cached.NetBiosName)) { + if (_domainInfoCache.TryGetValue(domainName, out var cached)) { return (true, cached); } @@ -1413,10 +1414,9 @@ internal static DomainInfo SelectRicherDomainInfo(DomainInfo seed, DomainInfo en /// private static async Task<(bool Success, DomainInfo DomainInfo)> ResolveDomainInfoCoreAsync( string domainName, ConnectionPoolManager pool, LdapConfig config, ILogger log) { - // Re-check inside the lazy: another caller may have published a satisfying record - // between our outer cache miss and this lazy's first execution. - if (_domainInfoCache.TryGetValue(domainName, out var cached) - && (pool == null || !string.IsNullOrEmpty(cached.NetBiosName))) { + // Re-check inside the lazy: another caller may have published a record between our + // outer cache miss and this lazy's first execution. + if (_domainInfoCache.TryGetValue(domainName, out var cached)) { return (true, cached); } @@ -2338,6 +2338,7 @@ public void ResetUtils() { _domainInfoCache.Clear(); _domainControllers.Clear(); lock (_currentDomainLock) { + _currentDomain?.Dispose(); _currentDomain = null; } LdapConnectionPool.ResetCaches(); From b9920b256ff425590af886742fe99553bbe11373 Mon Sep 17 00:00:00 2001 From: rvazarkar Date: Wed, 29 Apr 2026 13:22:51 -0400 Subject: [PATCH 19/20] feat: try multiple DCs for domain info enrichment --- src/CommonLib/LdapUtils.cs | 144 ++++++++++++++++++++++++------------- 1 file changed, 95 insertions(+), 49 deletions(-) diff --git a/src/CommonLib/LdapUtils.cs b/src/CommonLib/LdapUtils.cs index 8e5a37c65..345e48518 100644 --- a/src/CommonLib/LdapUtils.cs +++ b/src/CommonLib/LdapUtils.cs @@ -413,7 +413,7 @@ public IAsyncEnumerable> PagedQuery(LdapQueryParame } result = await Query(new LdapQueryParameters { - DomainName = domainInfo.Name, + DomainName = domainInfo.ForestName, Attributes = new[] { LDAPProperties.DistinguishedName, LDAPProperties.Name }, GlobalCatalog = true, LDAPFilter = new LdapFilter().AddFilter("(objectclass=trusteddomain)", true) @@ -1252,13 +1252,14 @@ DomainInfo PickRicher(string _, DomainInfo existing) /// /// Returns true when identifies the same domain that /// describes, comparing case-insensitively against - /// and . A candidate - /// with no is treated as unverifiable and accepted; the - /// resolution tiers always populate Name on success, so this branch only matters for - /// hand-built records in tests. + /// and . Candidates + /// with no are rejected: the resolution tiers contract is + /// to populate Name on every successful return, so a Name-less candidate is by definition + /// unverifiable and caching it under the caller's key would silently associate a broken + /// record with that domain. /// private static bool KeyMatchesCandidate(string key, DomainInfo candidate) { - if (string.IsNullOrEmpty(candidate.Name)) return true; + if (string.IsNullOrEmpty(candidate.Name)) return false; if (string.Equals(key, candidate.Name, StringComparison.OrdinalIgnoreCase)) return true; if (!string.IsNullOrEmpty(candidate.NetBiosName) && string.Equals(key, candidate.NetBiosName, StringComparison.OrdinalIgnoreCase)) return true; @@ -1280,11 +1281,14 @@ private static bool KeyMatchesCandidate(string key, DomainInfo candidate) { /// domain (cross-forest leak, decommissioned host, mismatched config.Server). In that /// case the seed is preferred even though the retry produced a higher score, because caching /// the retry's record under the seed's cache key would silently associate the wrong SID and - /// NetBIOS name with that domain. + /// NetBIOS name with that domain. A missing Name on either side is treated as a guard + /// failure (rather than letting string.Equals(null, null) wave through the merge), + /// since a Name-less record cannot be safely compared to anything. /// internal static DomainInfo SelectRicherDomainInfo(DomainInfo seed, DomainInfo enriched) { if (seed == null) return enriched; if (enriched == null) return seed; + if (string.IsNullOrEmpty(seed.Name) || string.IsNullOrEmpty(enriched.Name)) return seed; if (!string.Equals(seed.Name, enriched.Name, StringComparison.OrdinalIgnoreCase)) { return seed; } @@ -1545,8 +1549,8 @@ private static bool TryResolveHintViaUncontrolledGetDomain( if (!string.IsNullOrWhiteSpace(config.Server)) { log?.LogDebug( - "TryResolveHintViaUncontrolledGetDomain(\"{Name}\", out string DomainName) short-circuited: Specific Server is set", - domainName); + "TryResolveHintViaUncontrolledGetDomain short-circuited: Server={Server} is set", + config.Server); return false; } @@ -1625,7 +1629,11 @@ private static bool TryResolveHintViaUncontrolledGetDomain( } // Canonical name is always derivable from the default NC (e.g. DC=contoso,DC=local -> CONTOSO.LOCAL). - var name = Helpers.DistinguishedNameToDomain(defaultNc).ToUpper(); + // DistinguishedNameToDomain returns null for DNs without DC= components; a tier success contract + // requires a populated Name so reject the result rather than emitting a Name-less DomainInfo. + var derivedName = Helpers.DistinguishedNameToDomain(defaultNc); + if (string.IsNullOrEmpty(derivedName)) return (false, null); + var name = derivedName.ToUpper(); string domainSid = null; string forestName = null; string primaryDomainController = null; @@ -1816,14 +1824,12 @@ private static bool TryGetDomainInfoViaUncontrolledFallback(string domainName, L // Blocking External Call using var domain = Domain.GetDomain(context); - if (domain == null) { + if (domain == null || string.IsNullOrEmpty(domain.Name)) { return false; } - var name = domain.Name?.ToUpper(); - var distinguishedName = !string.IsNullOrEmpty(domain.Name) - ? Helpers.DomainNameToDistinguishedName(domain.Name) - : null; + var name = domain.Name.ToUpper(); + var distinguishedName = Helpers.DomainNameToDistinguishedName(domain.Name); string forestName = null; string primaryDomainController = null; string domainSid = null; @@ -1966,7 +1972,9 @@ private static bool TryGetDomainInfoViaUncontrolledFallback(string domainName, L return (false, null); } - name = Helpers.DistinguishedNameToDomain(defaultNc).ToUpper(); + var derivedName = Helpers.DistinguishedNameToDomain(defaultNc); + if (string.IsNullOrEmpty(derivedName)) return (false, null); + name = derivedName.ToUpper(); distinguishedName = defaultNc; if (root.TryGetSecurityIdentifier(out var sid) && !string.IsNullOrEmpty(sid)) { @@ -2140,54 +2148,86 @@ internal static string ResolveOneShotBindTarget(string domainName, LdapConfig co /// /// /// Skips the bind entirely when the seed already has a maximum - /// , when is set (the - /// server-pinning contract forbids binding to any host other than the configured one, - /// and consumers are warned that data loss is the intended trade-off), when no bind - /// target can be derived from the seed ( - /// falling back to the first entry of ), or - /// when the bind itself fails. In any of these cases the original seed is returned - /// unchanged. When the retry succeeds, the merged result is selected by + /// , when no bind target can be derived (no + /// pin, no , + /// and an empty list), or when every attempted + /// bind fails. In any of these cases the original seed is returned unchanged. + /// + /// Server pinning: when is set the pinned host is the + /// only enrichment target. Pinning's contract forbids fanning out to alternate DCs even + /// when the pinned target is unreachable, so a failed bind under pinning returns the + /// seed without trying any DC from the seed's discovery list. + /// + /// + /// Without pinning the function walks an ordered candidate list - PDC first, then each + /// entry of - case-insensitively deduplicated + /// and capped at targets so a stale or oversized + /// DC list cannot blow out call latency. The first successful resolve wins; subsequent + /// targets are not contacted. When the retry succeeds, the merged result is selected by /// , which enforces the canonical-name guard so a /// retry that lands on a DC outside the requested domain does not poison the cache. - /// : + /// + /// internal static async Task TryEnrichDomainInfoViaDirectLdapAsync( string domainName, DomainInfo seed, LdapConfig config, ILogger log) { if (seed == null) return null; if (CompletenessScore(seed) >= 7 || string.IsNullOrWhiteSpace(domainName) || config == null) return seed; - string target; + IReadOnlyList candidates; if (!string.IsNullOrWhiteSpace(config.Server)) { - target = config.Server; - } else if (!string.IsNullOrWhiteSpace(seed.PrimaryDomainController)) { - target = seed.PrimaryDomainController; - } else if (seed.DomainControllers is { Count: > 0 }) { - target = seed.DomainControllers[0]; + // Pinned: never fall through to the seed's discovery list. + candidates = new[] { config.Server }; } else { - target = null; + var ordered = new List(); + var seen = new HashSet(StringComparer.OrdinalIgnoreCase); + if (!string.IsNullOrWhiteSpace(seed.PrimaryDomainController) && + seen.Add(seed.PrimaryDomainController)) { + ordered.Add(seed.PrimaryDomainController); + } + if (seed.DomainControllers is { Count: > 0 }) { + foreach (var dc in seed.DomainControllers) { + if (string.IsNullOrWhiteSpace(dc) || !seen.Add(dc)) continue; + ordered.Add(dc); + if (ordered.Count >= MaxEnrichmentBindAttempts) break; + } + } + candidates = ordered; } - - if (string.IsNullOrWhiteSpace(target)) return seed; - var connection = TryBindOneShotLdapConnection(target, config, log); - if (connection == null) { + if (candidates.Count == 0) return seed; + + foreach (var target in candidates) { + var connection = TryBindOneShotLdapConnection(target, config, log); + if (connection == null) { + log?.LogDebug( + "Direct LDAP enrichment bind failed for {Domain} via {Target}", + domainName, target); + continue; + } + + var (ok, enriched) = await Task.Run(() => { + try { + return ResolveDomainInfoFromConnection(connection, domainName, log); + } + finally { + connection.Dispose(); + } + }).ConfigureAwait(false); + + if (ok) { + return SelectRicherDomainInfo(seed, enriched); + } + log?.LogDebug( - "Direct LDAP enrichment bind failed for {Domain} via discovered DC {Target}", + "Direct LDAP enrichment resolve failed for {Domain} via {Target}", domainName, target); - return seed; } - return await Task.Run(() => { - try { - var (ok, enriched) = ResolveDomainInfoFromConnection(connection, domainName, log); - if (!ok) return seed; - return SelectRicherDomainInfo(seed, enriched); - } - finally { - connection.Dispose(); - } - }).ConfigureAwait(false); + return seed; } + private const int MaxEnrichmentBindAttempts = 5; + /// /// Synchronous body of . Uses /// SendRequest directly rather than the pool's Query because no pool is @@ -2226,14 +2266,19 @@ private static (bool Success, DomainInfo DomainInfo) ResolveDomainInfoFromConnec return (false, null); } - var name = Helpers.DistinguishedNameToDomain(defaultNc).ToUpper(); + var derivedName = Helpers.DistinguishedNameToDomain(defaultNc); + if (string.IsNullOrEmpty(derivedName)) return (false, null); + var name = derivedName.ToUpper(); string domainSid = null; string forestName = null; string primaryDomainController = null; string netBiosName = null; IReadOnlyList domainControllers = null; if (!string.IsNullOrWhiteSpace(rootNc)) { - forestName = Helpers.DistinguishedNameToDomain(rootNc).ToUpper(); + var derivedForest = Helpers.DistinguishedNameToDomain(rootNc); + if (!string.IsNullOrEmpty(derivedForest)) { + forestName = derivedForest.ToUpper(); + } } // 2. Domain NC base search - objectSid + fsmoRoleOwner in one round-trip. @@ -2421,6 +2466,7 @@ private static PrincipalContext CreatePrincipalContext(LdapConfig config, string } public void Dispose() { + ResetUtils(); _connectionPool?.Dispose(); } From fbd67ea4ba559b0af163505fb828e4a34df170eb Mon Sep 17 00:00:00 2001 From: rvazarkar Date: Wed, 29 Apr 2026 13:40:22 -0400 Subject: [PATCH 20/20] chore: add a documented limit --- src/CommonLib/LdapUtils.cs | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/src/CommonLib/LdapUtils.cs b/src/CommonLib/LdapUtils.cs index 345e48518..154a7139f 100644 --- a/src/CommonLib/LdapUtils.cs +++ b/src/CommonLib/LdapUtils.cs @@ -1716,6 +1716,12 @@ private static bool TryResolveHintViaUncontrolledGetDomain( // DC enumeration uses the canonical "DC object = computer with SERVER_TRUST_ACCOUNT in UAC" // bit test (0x2000). This is the same filter used elsewhere in the project via CommonFilters. + // Uses pool.Query (non-paged) rather than pool.PagedQuery: a domain whose DC count exceeds + // the LDAP query policy's MaxPageSize (default 1000) would have its DomainControllers list + // silently truncated at MaxPageSize, since SendRequest returns SizeLimitExceeded as a result + // code rather than an exception and the pool's Query path does not inspect ResultCode. This + // is accepted: SharpHound's DC consumers only need a usable DC, not an exhaustive list, and + // forests with >MaxPageSize DCs are not in scope for this resolver. try { var dcs = new List(); var dcEnum = pool.Query(new LdapQueryParameters { @@ -2344,6 +2350,12 @@ private static (bool Success, DomainInfo DomainInfo) ResolveDomainInfoFromConnec } // 5. Domain controller enumeration - same filter as CommonFilters.DomainControllers. + // Single non-paged SendRequest: a domain whose DC count exceeds the LDAP query policy's + // MaxPageSize (default 1000) would have its DomainControllers list silently truncated - + // SizeLimitExceeded surfaces as a ResultCode rather than an exception, and the success + // path here yields whatever Entries the server returned. Matches the pool tier's + // non-paged DC enumeration on purpose (so the two tiers produce equivalent results) and + // is accepted for the same reason: forests with >MaxPageSize DCs are not in scope. try { var dcs = new List(); var dcReq = new SearchRequest(