Skip to content
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
7db7e42
feat: comprehensive replacement for the GetDomain calls to close one …
rvazarkar Apr 23, 2026
b78b939
fix: make DomainInfo immutable to prevent references being changed o…
rvazarkar Apr 23, 2026
78ddc21
chore: delete bad comments
rvazarkar Apr 23, 2026
0b839f8
chore: fix order of resolution in connection pool
rvazarkar Apr 27, 2026
2e5c059
fix: add enrichment of sparse tier domain resolution via LDAP retry
rvazarkar Apr 28, 2026
d3e697a
fix: several casing violations in various dictionaries that would lea…
rvazarkar Apr 28, 2026
6821eb0
fix: rewrap cache dictionaries as case insensitive after deserialization
rvazarkar Apr 28, 2026
5de8170
fix: valuetoidcache using poor casing leading to multiple cache entri…
rvazarkar Apr 28, 2026
ca5f73e
chore: fix cache misses on old GetDomain calls
rvazarkar Apr 28, 2026
a90d8a3
chore: properly dispose domain objects
rvazarkar Apr 28, 2026
6ba0ef4
chore: significantly simplify caching and domain resolution code
rvazarkar Apr 28, 2026
9c3f16c
fix: change sid/domain cache mapping so we dont accidentally poison t…
rvazarkar Apr 28, 2026
37004c8
chore: remove domain hint cache as we rarely call this function now …
rvazarkar Apr 28, 2026
7863689
fix: properly reset ExcludedDomains when resetting utils
rvazarkar Apr 28, 2026
6d2a1a4
fix: a potential crash in CopyCaseInsensitive
rvazarkar Apr 28, 2026
f543ceb
chore: simplify lazy logic significantly, exclude static callers from…
rvazarkar Apr 29, 2026
04a18f2
fix: significantly tighten up logic for checking if we're caching a D…
rvazarkar Apr 29, 2026
9062696
fix: move pool resolution above ADSI in GetDomainSidFromDomainName t…
rvazarkar Apr 29, 2026
b9920b2
feat: try multiple DCs for domain info enrichment
rvazarkar Apr 29, 2026
fbd67ea
chore: add a documented limit
rvazarkar Apr 29, 2026
f14cd22
fix: call ResetUtils when setting the LDAP config, clear inflight res…
rvazarkar May 6, 2026
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 13 additions & 13 deletions src/CommonLib/ConnectionPoolManager.cs
Original file line number Diff line number Diff line change
@@ -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;
Expand Down Expand Up @@ -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);
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.

foreach (var name in _translateNames)
try {
Expand Down
52 changes: 52 additions & 0 deletions src/CommonLib/DomainInfo.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
using System;
using System.Collections.Generic;

namespace SharpHoundCommonLib
{
/// <summary>
/// Lightweight, transport-agnostic description of an Active Directory domain populated either
/// from controlled LDAP queries (honoring <see cref="LdapConfig"/>) or, when explicitly opted in
/// via <see cref="LdapConfig.AllowFallbackToUncontrolledLdap"/>, from
/// <c>System.DirectoryServices.ActiveDirectory.Domain.GetDomain</c>.
/// </summary>
public sealed class DomainInfo
{
/// <summary>Upper-cased DNS name of the domain (e.g. <c>CONTOSO.LOCAL</c>).</summary>
public string Name { get; }

/// <summary>Default naming context distinguished name (e.g. <c>DC=contoso,DC=local</c>).</summary>
public string DistinguishedName { get; }

/// <summary>Upper-cased DNS name of the forest root domain, when known.</summary>
public string ForestName { get; }

/// <summary>Domain SID (S-1-5-21-...) if resolved, otherwise null.</summary>
public string DomainSid { get; }

/// <summary>Legacy NetBIOS domain name if resolved from the Partitions container, otherwise null.</summary>
public string NetBiosName { get; }

/// <summary>DNS hostname of the PDC FSMO role owner if resolved, otherwise null.</summary>
public string PrimaryDomainController { get; }

/// <summary>DNS hostnames of known domain controllers for this domain.</summary>
public IReadOnlyList<string> DomainControllers { get; }

public DomainInfo(
string name = null,
string distinguishedName = null,
string forestName = null,
string domainSid = null,
string netBiosName = null,
string primaryDomainController = null,
IReadOnlyList<string> domainControllers = null) {
Name = name;
DistinguishedName = distinguishedName;
ForestName = forestName;
DomainSid = domainSid;
NetBiosName = netBiosName;
PrimaryDomainController = primaryDomainController;
DomainControllers = domainControllers ?? Array.Empty<string>();
}
}
}
2 changes: 2 additions & 0 deletions src/CommonLib/Enums/LDAPProperties.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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";
}
}
19 changes: 19 additions & 0 deletions src/CommonLib/ILdapUtils.cs
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,25 @@ IAsyncEnumerable<Result<string>> RangedRetrieval(string distinguishedName,
/// <returns>True if the domain was found, false if not</returns>
bool GetDomain(out System.DirectoryServices.ActiveDirectory.Domain domain);

/// <summary>
/// Resolves a <see cref="DomainInfo"/> for the specified domain using controlled LDAP queries
/// that honor the configured <see cref="LdapConfig"/> (server, port, SSL, auth, signing, cert verification).
/// Falls back to <c>System.DirectoryServices.ActiveDirectory.Domain.GetDomain</c> only when
/// <see cref="LdapConfig.AllowFallbackToUncontrolledLdap"/> is enabled.
/// </summary>
/// <param name="domainName">The domain name to resolve</param>
/// <returns>A tuple containing success state as well as the populated DomainInfo if successful</returns>
Task<(bool Success, DomainInfo DomainInfo)> GetDomainInfoAsync(string domainName);

/// <summary>
/// Resolves a <see cref="DomainInfo"/> for the user's current domain using controlled LDAP queries
/// that honor the configured <see cref="LdapConfig"/>. Falls back to
/// <c>System.DirectoryServices.ActiveDirectory.Domain.GetDomain</c> only when
/// <see cref="LdapConfig.AllowFallbackToUncontrolledLdap"/> is enabled.
/// </summary>
/// <returns>A tuple containing success state as well as the populated DomainInfo if successful</returns>
Task<(bool Success, DomainInfo DomainInfo)> GetDomainInfoAsync();
Comment on lines +92 to +109
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

This public interface change is source-breaking.

Adding two required members to ILdapUtils will break any downstream implementation at compile time, so this cannot be treated as a non-breaking feature. If compatibility matters, introduce a new interface or adapter for domain-info resolution instead of extending the existing public contract.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/CommonLib/ILdapUtils.cs` around lines 92 - 109, The added methods
GetDomainInfoAsync(string) and GetDomainInfoAsync() on ILdapUtils are
source-breaking; instead of modifying ILdapUtils, create a new interface (e.g.
IDomainInfoResolver or ILdapDomainResolver) that declares Task<(bool Success,
DomainInfo DomainInfo)> GetDomainInfoAsync(string) and Task<(bool Success,
DomainInfo DomainInfo)> GetDomainInfoAsync(), implement that new interface in
the classes that provide domain resolution, and update callers that need this
functionality to depend on the new interface rather than changing the existing
ILdapUtils contract.


Task<(bool Success, string ForestName)> GetForest(string domain);
/// <summary>
/// Attempts to resolve an account name to its corresponding typed principal
Expand Down
6 changes: 6 additions & 0 deletions src/CommonLib/LdapConfig.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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}");
}
Expand Down
68 changes: 40 additions & 28 deletions src/CommonLib/LdapConnectionPool.cs
Original file line number Diff line number Diff line change
@@ -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;
Expand Down Expand Up @@ -451,7 +450,7 @@ private async Task<LdapQuerySetupResult> 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";
Expand Down Expand Up @@ -492,7 +491,7 @@ public async IAsyncEnumerable<Result<string>> 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<string>.Fail("Failed to create search request");
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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)) {
Expand All @@ -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, "");
Comment thread
coderabbitai[bot] marked this conversation as resolved.
}
}
Expand Down
Loading
Loading