Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
20 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
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
151 changes: 140 additions & 11 deletions src/CommonLib/Cache.cs
Original file line number Diff line number Diff line change
Expand Up @@ -24,11 +24,11 @@ public class Cache

private Cache()
{
ValueToIdCache = new ConcurrentDictionary<string, string>();
ValueToIdCache = new ConcurrentDictionary<string, string>(StringComparer.OrdinalIgnoreCase);
IdToTypeCache = new ConcurrentDictionary<string, Label>();
GlobalCatalogCache = new ConcurrentDictionary<string, string[]>();
GlobalCatalogCache = new ConcurrentDictionary<string, string[]>(StringComparer.OrdinalIgnoreCase);
MachineSidCache = new ConcurrentDictionary<string, string>();
SIDToDomainCache = new ConcurrentDictionary<string, string>();
SIDToDomainCache = new ConcurrentDictionary<string, string>(StringComparer.OrdinalIgnoreCase);
}

[DataMember] public ConcurrentDictionary<string, string[]> GlobalCatalogCache { get; private set; }
Expand All @@ -46,13 +46,96 @@ private Cache()
[IgnoreDataMember] private static Cache CacheInstance { get; set; }

/// <summary>
/// Add a SID to/from Domain mapping to the cache
/// 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.
/// </summary>
/// <param name="key"></param>
/// <param name="value"></param>
/// <param name="key">A SID or a domain name.</param>
/// <param name="value">The corresponding domain name or SID.</param>
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;

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);
}

// 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)
{
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;
}

/// <summary>
Expand Down Expand Up @@ -102,7 +185,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;
}
Expand Down Expand Up @@ -151,9 +234,52 @@ public static Cache CreateNewCache(Version version = null)
public static void SetCacheInstance(Cache cache)
{
CacheInstance = cache;
NormalizeCaseInsensitiveCaches();
CreateMissingDictionaries();
}

/// <summary>
/// Rewraps dictionaries that must be case-insensitive after assignment. Serializers
/// (DataContractSerializer, Newtonsoft.Json, System.Text.Json) reconstruct
/// <see cref="ConcurrentDictionary{TKey,TValue}"/> 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.
/// </summary>
private static void NormalizeCaseInsensitiveCaches()
{
if (CacheInstance == null) return;
if (CacheInstance.SIDToDomainCache != null)
{
CacheInstance.SIDToDomainCache = CopyCaseInsensitive(CacheInstance.SIDToDomainCache);
}
if (CacheInstance.GlobalCatalogCache != null)
{
CacheInstance.GlobalCatalogCache = CopyCaseInsensitive(CacheInstance.GlobalCatalogCache);
}
if (CacheInstance.ValueToIdCache != null)
{
CacheInstance.ValueToIdCache = CopyCaseInsensitive(CacheInstance.ValueToIdCache);
}
}

/// <summary>
/// Copies <paramref name="source"/> into a new <see cref="ConcurrentDictionary{TKey,TValue}"/> keyed
/// by <see cref="StringComparer.OrdinalIgnoreCase"/>. 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.
/// </summary>
private static ConcurrentDictionary<string, TValue> CopyCaseInsensitive<TValue>(
ConcurrentDictionary<string, TValue> source)
{
var copy = new ConcurrentDictionary<string, TValue>(StringComparer.OrdinalIgnoreCase);
foreach (var kvp in source)
{
copy.TryAdd(kvp.Key, kvp.Value);
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
return copy;
}

/// <summary>
/// Gets stats from the currently loaded cache
/// </summary>
Expand Down Expand Up @@ -184,10 +310,13 @@ private static void CreateMissingDictionaries()
{
CacheInstance ??= new Cache();
CacheInstance.IdToTypeCache ??= new ConcurrentDictionary<string, Label>();
CacheInstance.GlobalCatalogCache ??= new ConcurrentDictionary<string, string[]>();
CacheInstance.GlobalCatalogCache ??=
new ConcurrentDictionary<string, string[]>(StringComparer.OrdinalIgnoreCase);
CacheInstance.MachineSidCache ??= new ConcurrentDictionary<string, string>();
CacheInstance.SIDToDomainCache ??= new ConcurrentDictionary<string, string>();
CacheInstance.ValueToIdCache ??= new ConcurrentDictionary<string, string>();
CacheInstance.SIDToDomainCache ??=
new ConcurrentDictionary<string, string>(StringComparer.OrdinalIgnoreCase);
CacheInstance.ValueToIdCache ??=
new ConcurrentDictionary<string, string>(StringComparer.OrdinalIgnoreCase);
}
}
}
37 changes: 23 additions & 14 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 @@ -135,6 +133,29 @@ private string ResolveIdentifier(string identifier) {
private (bool, string) GetDomainSidFromDomainName(string domainName) {
if (Cache.GetDomainSidMapping(domainName, out var domainSid)) return (true, domainSid);

// 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();
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);
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.

try {
var entry = Helpers.CreateDirectoryEntry($"LDAP://{domainName}", _ldapConfig);
if (entry.TryGetSecurityIdentifier(out var sid)) {
Expand All @@ -146,18 +167,6 @@ 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)
}

foreach (var name in _translateNames)
try {
var account = new NTAccount(domainName, name);
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
Loading
Loading