feat: comprehensive replacement for GetDomain with fallbacks#299
feat: comprehensive replacement for GetDomain with fallbacks#299rvazarkar wants to merge 3 commits intoldap_beta_fixesfrom
Conversation
…of the biggest ADSI gaps, as well as several improvements to the logic
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThe pull request introduces an async, controlled domain-resolution pipeline with a new Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
test/unit/Facades/MockLdapUtils.cs (1)
706-712: Consider returning a baselineDomainInfofrom the test double.This mock already models a default
TESTLAB.LOCALenvironment viaGetForest,GetDomainSidFromDomainName, and the identity resolvers. Returning(false, null)here makes the new API the only domain-resolution path that fails by default, which can turn unrelated tests red as production code migrates fromGetDomain*toGetDomainInfoAsync(). A minimal happy-pathDomainInfowould keep the base fixture behavior consistent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/Facades/MockLdapUtils.cs` around lines 706 - 712, The mock's GetDomainInfoAsync overloads return (false, null) which breaks the established TESTLAB.LOCAL baseline used by GetForest and GetDomainSidFromDomainName; update both GetDomainInfoAsync(string) and parameterless GetDomainInfoAsync() to return Task.FromResult((true, baselineDomainInfo)) where baselineDomainInfo is a minimal DomainInfo instance that matches the existing test fixture (e.g., DomainName "TESTLAB.LOCAL", the same ForestName and DomainSid used by GetForest/GetDomainSidFromDomainName and any identity resolvers) so tests continue to see a successful, consistent domain resolution path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/CommonLib/DomainInfo.cs`:
- Around line 12-33: DomainInfo instances are mutable and are being returned
from a static cache in LdapUtils.cs, allowing callers to mutate shared cached
objects; make DomainInfo immutable (remove public setters on Name,
DistinguishedName, ForestName, DomainSid, NetBiosName, PrimaryDomainController
and DomainControllers) and provide a single constructor (or init-only
properties) that fully initializes all properties (keeping DomainControllers as
an IReadOnlyList<string> defaulting to Array.Empty<string>()), then update
LdapUtils.cs to only store and return immutable DomainInfo objects (or to
clone/create a new DomainInfo instance at the cache boundary when
reading/writing) so cached state cannot be mutated by consumers.
In `@src/CommonLib/LdapUtils.cs`:
- Around line 35-36: The static cache _domainInfoCache currently keys only by
domain and can return entries produced by
TryGetDomainInfoViaUncontrolledFallback even when
LdapConfig.AllowFallbackToUncontrolledLdap is false; fix by encoding fallback
provenance in the cache key or marking cached DomainInfo entries with a boolean
(e.g., DomainInfo.IsFallback) and, at every cache read site (places that read
_domainInfoCache and return cached values, and where
TryGetDomainInfoViaUncontrolledFallback writes into the cache), revalidate
against LdapConfig.AllowFallbackToUncontrolledLdap—only return a cached
IsFallback entry when AllowFallbackToUncontrolledLdap is true, otherwise bypass
the cached fallback entry and proceed to controlled lookup or refresh the cache
entry appropriately.
- Around line 1456-1457: The XML doc comments in LdapUtils.cs reference a
non-existent member DomainInfo.FromFallback which causes unresolved cref
warnings; fix by either adding a boolean member/property named FromFallback to
the DomainInfo class (and document it) or update the XML docs to remove or
replace the cref with an existing member (e.g., an actual property like
DomainInfo.IsFallback or simply remove the cref). Update all occurrences that
reference DomainInfo.FromFallback so the cref targets a real symbol or is
removed to eliminate the unresolved-reference warnings.
---
Nitpick comments:
In `@test/unit/Facades/MockLdapUtils.cs`:
- Around line 706-712: The mock's GetDomainInfoAsync overloads return (false,
null) which breaks the established TESTLAB.LOCAL baseline used by GetForest and
GetDomainSidFromDomainName; update both GetDomainInfoAsync(string) and
parameterless GetDomainInfoAsync() to return Task.FromResult((true,
baselineDomainInfo)) where baselineDomainInfo is a minimal DomainInfo instance
that matches the existing test fixture (e.g., DomainName "TESTLAB.LOCAL", the
same ForestName and DomainSid used by GetForest/GetDomainSidFromDomainName and
any identity resolvers) so tests continue to see a successful, consistent domain
resolution path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 21e5791e-c2db-494d-afb2-12809aeda7a6
📒 Files selected for processing (11)
src/CommonLib/ConnectionPoolManager.cssrc/CommonLib/DomainInfo.cssrc/CommonLib/Enums/LDAPProperties.cssrc/CommonLib/ILdapUtils.cssrc/CommonLib/LdapConfig.cssrc/CommonLib/LdapConnectionPool.cssrc/CommonLib/LdapUtils.cssrc/CommonLib/Processors/GPOLocalGroupProcessor.cstest/unit/Facades/MockLdapUtils.cstest/unit/GPOLocalGroupProcessorTest.cstest/unit/LDAPUtilsTest.cs
| public sealed class DomainInfo | ||
| { | ||
| /// <summary>Upper-cased DNS name of the domain (e.g. <c>CONTOSO.LOCAL</c>).</summary> | ||
| public string Name { get; set; } | ||
|
|
||
| /// <summary>Default naming context distinguished name (e.g. <c>DC=contoso,DC=local</c>).</summary> | ||
| public string DistinguishedName { get; set; } | ||
|
|
||
| /// <summary>Upper-cased DNS name of the forest root domain, when known.</summary> | ||
| public string ForestName { get; set; } | ||
|
|
||
| /// <summary>Domain SID (S-1-5-21-...) if resolved, otherwise null.</summary> | ||
| public string DomainSid { get; set; } | ||
|
|
||
| /// <summary>Legacy NetBIOS domain name if resolved from the Partitions container, otherwise null.</summary> | ||
| public string NetBiosName { get; set; } | ||
|
|
||
| /// <summary>DNS hostname of the PDC FSMO role owner if resolved, otherwise null.</summary> | ||
| public string PrimaryDomainController { get; set; } | ||
|
|
||
| /// <summary>DNS hostnames of known domain controllers for this domain.</summary> | ||
| public IReadOnlyList<string> DomainControllers { get; set; } = Array.Empty<string>(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Avoid caching a mutable public model by reference.
src/CommonLib/LdapUtils.cs stores DomainInfo instances in a static cache and returns the same reference to every caller. With public setters on every property here, any consumer can mutate a cached instance and affect later lookups process-wide. Please make the model immutable, or clone it at the cache boundary before this ships.
Based on learnings, public properties added in SharpHoundCommon may be consumed by dependent projects even when not used within this repository itself.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/CommonLib/DomainInfo.cs` around lines 12 - 33, DomainInfo instances are
mutable and are being returned from a static cache in LdapUtils.cs, allowing
callers to mutate shared cached objects; make DomainInfo immutable (remove
public setters on Name, DistinguishedName, ForestName, DomainSid, NetBiosName,
PrimaryDomainController and DomainControllers) and provide a single constructor
(or init-only properties) that fully initializes all properties (keeping
DomainControllers as an IReadOnlyList<string> defaulting to
Array.Empty<string>()), then update LdapUtils.cs to only store and return
immutable DomainInfo objects (or to clone/create a new DomainInfo instance at
the cache boundary when reading/writing) so cached state cannot be mutated by
consumers.
| /// <see cref="DomainInfo"/> with <see cref="DomainInfo.FromFallback"/> = false. A failure | ||
| /// to acquire the connection or to read the default NC is fatal and returns <c>(false, null)</c>. |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd -e csproj -e props -e targets | xargs -r rg -n "GenerateDocumentationFile|TreatWarningsAsErrors|WarningsAsErrors"
rg -n "DomainInfo\\.FromFallback|<see cref=\"DomainInfo\\.FromFallback\""Repository: SpecterOps/SharpHoundCommon
Length of output: 332
🏁 Script executed:
fd -e cs -i "*domaininfo*" | head -20Repository: SpecterOps/SharpHoundCommon
Length of output: 362
🏁 Script executed:
cat -n src/CommonLib/DomainInfo.cs | head -100Repository: SpecterOps/SharpHoundCommon
Length of output: 1916
🏁 Script executed:
rg -n "FromFallback" src/CommonLib/Repository: SpecterOps/SharpHoundCommon
Length of output: 332
The XML docs reference a member that doesn't exist.
Both comments at lines 1456 and 1630 point to DomainInfo.FromFallback, but the DomainInfo class has no such member. These broken crefs will raise unresolved-reference warnings when XML docs are generated. Either add the missing member or remove the cref references.
Also applies to: 1629-1630
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/CommonLib/LdapUtils.cs` around lines 1456 - 1457, The XML doc comments in
LdapUtils.cs reference a non-existent member DomainInfo.FromFallback which
causes unresolved cref warnings; fix by either adding a boolean member/property
named FromFallback to the DomainInfo class (and document it) or update the XML
docs to remove or replace the cref with an existing member (e.g., an actual
property like DomainInfo.IsFallback or simply remove the cref). Update all
occurrences that reference DomainInfo.FromFallback so the cref targets a real
symbol or is removed to eliminate the unresolved-reference warnings.
1 participant
Description
Addresses one of the main offenders of uncontrolled LDAP calls in the codebase. Also fixes several gaps in netonly scenarios, and drastically improves domain info resolution with several fallbacks. Adds an optional flag to the LDAPConfig which allows
Motivation and Context
This PR addresses: [GitHub issue or Jira ticket number]
How Has This Been Tested?
Unit tests for many of the changes, real testing to follow.
Screenshots (if appropriate):
Types of changes
Checklist:
Summary by CodeRabbit
Release Notes
New Features
AllowFallbackToUncontrolledLdapconfiguration option to control LDAP fallback behavior.CurrentUserDomainconfiguration option for domain specification.Refactor