Feature: Add collection of Active Directory Sites ACLs and data#257
Feature: Add collection of Active Directory Sites ACLs and data#257q-roland wants to merge 10 commits into
Conversation
…ition * Collect data related to AD sites * Collect data related to AD site subnets - add containedBy attribute for related site * Collect data related to AD site servers - add containedBy attribute for related site
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughAdds Active Directory Site, SiteServer, and SiteSubnet support: new enums/constants, LDAP filters and property arrays, SiteProcessor for DN/site resolution and GPLink parsing, ACL adjustments, new output fields, and comprehensive unit tests. ChangesSite support
sequenceDiagram
participant Collector
participant LdapProducerQueryGenerator
participant LdapFilter
participant LdapUtils
participant SiteProcessor
participant OutputTypes
Collector->>LdapProducerQueryGenerator: generate queries with Site flag
activate LdapProducerQueryGenerator
LdapProducerQueryGenerator->>LdapFilter: AddSites/AddSiteServers/AddSiteSubnets
LdapProducerQueryGenerator->>Collector: return site queries
deactivate LdapProducerQueryGenerator
Collector->>LdapUtils: query and resolve site objects
activate LdapUtils
LdapUtils->>LdapUtils: ResolveLabel -> Site/SiteServer/SiteSubnet
LdapUtils->>LdapUtils: ComputeDisplayName per label
LdapUtils->>Collector: return resolved results
deactivate LdapUtils
Collector->>SiteProcessor: GetContainingSiteForServer/Subnet and ReadSiteGPLinks
activate SiteProcessor
SiteProcessor->>LdapUtils: ResolveDistinguishedName for sites and GPLink targets
SiteProcessor->>Collector: yield GPLink and containing site principals
deactivate SiteProcessor
Collector->>OutputTypes: populate Site/SiteServer/SiteSubnet outputs
activate OutputTypes
OutputTypes->>Collector: return typed output
deactivate OutputTypes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/CommonLib/LdapProducerQueryGenerator.cs (1)
107-110: LGTM!The Site collection method is properly integrated into the configuration partition query generation. The condition correctly checks for the Site flag, and the filter chain appropriately includes site-related object types.
Optional readability suggestion: Line 110 contains a long method chain (8 methods). Consider breaking it across multiple lines for improved readability:
- filter = filter.AddContainers().AddConfiguration().AddCertificateTemplates().AddCertificateAuthorities() - .AddEnterpriseCertificationAuthorities().AddIssuancePolicies().AddSites().AddSiteServers().AddSiteSubnets(); + filter = filter.AddContainers() + .AddConfiguration() + .AddCertificateTemplates() + .AddCertificateAuthorities() + .AddEnterpriseCertificationAuthorities() + .AddIssuancePolicies() + .AddSites() + .AddSiteServers() + .AddSiteSubnets();src/CommonLib/OutputTypes/Site.cs (1)
7-9: Consider removing or documenting commented code.The commented-out
SubnetsandServersproperties could be handled in one of these ways:
- If these are planned for near-term implementation: Add a TODO comment instead of leaving commented code:
// TODO: Add Subnets and Servers properties to represent site children public GPLink[] Links { get; set; } = Array.Empty<GPLink>();
If these are optional future features: Remove the commented code to keep the codebase clean. Document the potential extension points in XML comments or separate documentation if needed.
If implementation is imminent: Uncomment and implement them as part of this PR to align with the containedBy relationships mentioned in the PR description.
Commented code can make maintenance more difficult and may cause confusion about whether it represents unfinished work or optional features.
src/CommonLib/LdapUtils.cs (1)
1423-1434: Prefer DNS host name for SiteServer displayUsing the
nameattribute alone hides the fully qualified host information and can collide in multi-domain forests.serverobjects exposedNSHostName, so we can surface the FQDN when it’s present.(learn.microsoft.com)- if (directoryObject.TryGetProperty(LDAPProperties.Name, out var name)) - { - displayName = $"{name}"; - } - else - { - displayName = $"UNKNOWN"; - } + if (directoryObject.TryGetProperty(LDAPProperties.DNSHostName, out var dnsHostName) && + !string.IsNullOrWhiteSpace(dnsHostName)) + { + displayName = dnsHostName; + } + else if (directoryObject.TryGetProperty(LDAPProperties.Name, out var name)) + { + displayName = name; + } + else + { + displayName = "UNKNOWN"; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
src/CommonLib/Enums/CollectionMethod.cs(1 hunks)src/CommonLib/Enums/DataType.cs(1 hunks)src/CommonLib/Enums/LDAPProperties.cs(1 hunks)src/CommonLib/Enums/Labels.cs(1 hunks)src/CommonLib/Enums/ObjectClass.cs(1 hunks)src/CommonLib/LdapProducerQueryGenerator.cs(2 hunks)src/CommonLib/LdapQueries/CommonProperties.cs(1 hunks)src/CommonLib/LdapQueries/LdapFilter.cs(1 hunks)src/CommonLib/LdapUtils.cs(4 hunks)src/CommonLib/OutputTypes/Site.cs(1 hunks)src/CommonLib/OutputTypes/SiteServer.cs(1 hunks)src/CommonLib/OutputTypes/SiteSubnet.cs(1 hunks)src/CommonLib/Processors/ACLProcessor.cs(3 hunks)src/CommonLib/Processors/LdapPropertyProcessor.cs(1 hunks)src/CommonLib/Processors/SiteProcessor.cs(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-17T13:43:46.833Z
Learnt from: MikeX777
Repo: SpecterOps/SharpHoundCommon PR: 241
File: src/CommonLib/Processors/LdapPropertyProcessor.cs:168-169
Timestamp: 2025-10-17T13:43:46.833Z
Learning: Properties added to dictionaries returned by methods in SharpHoundCommon (such as those in LdapPropertyProcessor) may be consumed by dependent projects like SharpHound (SH) and SharpHoundEnterprise (SHE), even if they are not used within the SharpHoundCommon repository itself.
Applied to files:
src/CommonLib/LdapQueries/CommonProperties.cssrc/CommonLib/Processors/LdapPropertyProcessor.cssrc/CommonLib/Enums/LDAPProperties.cs
🔇 Additional comments (9)
src/CommonLib/Enums/LDAPProperties.cs (1)
99-100: LGTM!The new LDAP property constants follow the established naming conventions and pattern. They integrate cleanly with the site-related features introduced in this PR.
src/CommonLib/OutputTypes/SiteSubnet.cs (1)
1-7: Verify containedBy relationship implementation for subnets.The PR description states that subnets should have a "containedBy attribute linking each subnet to its site," but the
SiteSubnetclass doesn't contain any properties. Please confirm whether:
- The containedBy relationship is implemented through the base
OutputBaseclass- The relationship is stored separately (e.g., in a different data structure)
- This property is still pending implementation
If the containedBy relationship is not yet implemented, please ensure it's added to properly represent the subnet-to-site linkage as described in the PR objectives.
src/CommonLib/Enums/DataType.cs (1)
18-20: LGTM!The new data type constants are consistent with existing patterns and properly support the site-related collection features.
src/CommonLib/OutputTypes/SiteServer.cs (1)
1-7: Verify containedBy relationship implementation for servers.Similar to
SiteSubnet, the PR description mentions that servers should have a "containedBy attribute linking each server to its site," but theSiteServerclass is empty. Please confirm whether:
- The containedBy relationship is implemented through the base
OutputBaseclass- The relationship is stored separately
- This property is still pending implementation
If not yet implemented, please add the containedBy property to represent the server-to-site linkage as outlined in the PR objectives.
src/CommonLib/Enums/CollectionMethod.cs (2)
30-30: LGTM!The Site flag is correctly positioned at bit 24 (next available bit) and follows the established bitwise flag pattern.
37-38: LGTM!Site is appropriately included in the Default collection method composite, making site collection enabled by default alongside other standard collection methods.
src/CommonLib/LdapProducerQueryGenerator.cs (1)
135-140: LGTM!Site-related properties are correctly added when the Site collection flag is set, following the same pattern as other collection types in this method.
src/CommonLib/Enums/Labels.cs (1)
22-24: LGTM!The new site-related label enum members are properly added and follow the existing conventions. They integrate well with the site collection features throughout the PR.
src/CommonLib/OutputTypes/Site.cs (1)
10-10: LGTM!The
Linksproperty is properly implemented with a safe default value usingArray.Empty<GPLink>(), which is more efficient than allocating a new empty array.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/unit/ACLProcessorTest.cs (1)
2158-2308: ⚡ Quick winAdd coverage for the remaining new ACL branches (
Label.SiteGPLink andMembershipPropertySet).The suite now covers
SiteServer/SiteSubnetearly-exit andWriteAltSecurityIdentities/WritePublicInformation, but it still misses two newly introduced paths insrc/CommonLib/Processors/ACLProcessor.cs:Label.Site + ACEGuids.WriteGPLinkandACEGuids.MembershipPropertySetmapping toAddSelf/AddMember. Adding those assertions will lock in the new behavior and prevent silent regressions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/unit/ACLProcessorTest.cs` around lines 2158 - 2308, Add unit tests to cover the missing ACLProcessor branches for Label.Site and MembershipPropertySet: create tests similar to the existing GenericWrite cases but call ProcessACL with Label.Site and set the mock rule.ObjectType() to new Guid(ACEGuids.WriteGPLink) to assert the rightName maps to the GPLink behavior; also add tests where ObjectType() returns new Guid(ACEGuids.MembershipPropertySet) and assert the produced RightName is AddSelf or AddMember as appropriate, using the same mocking pattern (ILdapUtils, ActiveDirectorySecurityDescriptor, ActiveDirectoryRuleDescriptor) and assertions as the WriteAltSecurityIdentities/WritePublicInformation tests to lock in the new behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@test/unit/ACLProcessorTest.cs`:
- Around line 2158-2308: Add unit tests to cover the missing ACLProcessor
branches for Label.Site and MembershipPropertySet: create tests similar to the
existing GenericWrite cases but call ProcessACL with Label.Site and set the mock
rule.ObjectType() to new Guid(ACEGuids.WriteGPLink) to assert the rightName maps
to the GPLink behavior; also add tests where ObjectType() returns new
Guid(ACEGuids.MembershipPropertySet) and assert the produced RightName is
AddSelf or AddMember as appropriate, using the same mocking pattern (ILdapUtils,
ActiveDirectorySecurityDescriptor, ActiveDirectoryRuleDescriptor) and assertions
as the WriteAltSecurityIdentities/WritePublicInformation tests to lock in the
new behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 95ddb787-485c-4c6d-8f6e-e51a5898fd10
📒 Files selected for processing (8)
.gitignoresrc/CommonLib/Enums/Labels.cssrc/CommonLib/LdapUtils.cssrc/CommonLib/Processors/ACLProcessor.cssrc/CommonLib/Processors/LdapPropertyProcessor.cssrc/CommonLib/Processors/SiteProcessor.cstest/unit/ACLProcessorTest.cstest/unit/SiteProcessorTest.cs
✅ Files skipped from review due to trivial changes (1)
- .gitignore
🚧 Files skipped from review as they are similar to previous changes (4)
- src/CommonLib/Enums/Labels.cs
- src/CommonLib/Processors/LdapPropertyProcessor.cs
- src/CommonLib/Processors/SiteProcessor.cs
- src/CommonLib/LdapUtils.cs
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/CommonLib/Processors/SiteProcessor.cs (2)
86-100:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate that containment resolution actually returns a
Site.Line 90 and Line 100 currently treat any resolved DN as a valid containing site. That can emit a
containedByedge to a non-site object if the DN is malformed or resolves unexpectedly, which breaks the new site graph contract.Proposed fix
public async Task<(bool Success, TypedPrincipal Principal)> GetContainingSiteForServer(string distinguishedName) { var servercontainerdn = Helpers.RemoveDistinguishedNamePrefix(distinguishedName); var sitedn = Helpers.RemoveDistinguishedNamePrefix(servercontainerdn); - return await _utils.ResolveDistinguishedName(sitedn); + var resolved = await _utils.ResolveDistinguishedName(sitedn); + return resolved.Success && resolved.Principal?.ObjectType == Label.Site + ? resolved + : (false, default); } @@ public async Task<(bool Success, TypedPrincipal Principal)> GetContainingSiteForSubnet(string siteObject) { - return await _utils.ResolveDistinguishedName(siteObject); + var resolved = await _utils.ResolveDistinguishedName(siteObject); + return resolved.Success && resolved.Principal?.ObjectType == Label.Site + ? resolved + : (false, default); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/CommonLib/Processors/SiteProcessor.cs` around lines 86 - 100, The resolution methods currently return whatever _utils.ResolveDistinguishedName yields without verifying the result is actually a Site; update GetContainingSiteForServer and GetContainingSiteForSubnet to call _utils.ResolveDistinguishedName as now, then validate the returned TypedPrincipal represents a Site (e.g., check TypedPrincipal.Type or ObjectClass/IsSite property on the returned Principal). If the resolved principal is not a Site, return (false, default(TypedPrincipal)) (or the existing failure tuple pattern) so callers don’t create containedBy edges to non-site objects. Ensure both methods use the same validation logic after calling _utils.ResolveDistinguishedName.
139-151:⚠️ Potential issue | 🟠 Major | ⚡ Quick winOnly emit GPLinks when the resolved object is a GPO.
Line 145 only checks
Success. IfResolveDistinguishedNamereturns some other label, this will still serialize aGPLinkedge with a non-GPO target.Proposed fix
- if (res.Success) + if (res.Success && res.Principal?.ObjectType == Label.GPO) { yield return new GPLink { GUID = res.Principal.ObjectIdentifier, IsEnforced = enforced🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/CommonLib/Processors/SiteProcessor.cs` around lines 139 - 151, The loop currently yields GPLink whenever _utils.ResolveDistinguishedName(link.DistinguishedName) returns res.Success, which can produce edges for non-GPOs; update the logic in the foreach over Helpers.SplitGPLinkProperty(gpLink) to additionally verify the resolved principal is a GPO (e.g., inspect res.Principal.ObjectClass or res.Principal.Label for the GPO identifier used in your model) before yielding a GPLink with res.Principal.ObjectIdentifier and IsEnforced; only emit the GPLink when both res.Success is true and the principal indicates a GPO.
🧹 Nitpick comments (3)
src/CommonLib/Enums/EdgeNames.cs (1)
27-27: Reassess edge-name risk:SeverIsappears to be the intentional contract spelling
EdgeNames.SeverIs("SeverIs") is matched bySiteServer.SeverIs(unit test assertsEdgeNames.SeverIs == nameof(SiteServer.SeverIs)), so this repo’s internal contract is consistent.- No
ServerIsidentifier/literal is present here, so the typo risk is likely limited to external schema/paired components—confirm the expected edge-name string in the BloodHound/SharpHound ingestion/query schema.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/CommonLib/Enums/EdgeNames.cs` at line 27, EdgeNames defines a constant EdgeNames.SeverIs ("SeverIs") that matches SiteServer.SeverIs in tests, but this looks like a potential typo vis-à-vis external BloodHound/SharpHound schemas; verify which literal the external schema expects and then either (A) if the external contract truly uses "SeverIs", add a clarifying comment and keep the constant as-is (or add an explicit alias const string ServerIs = EdgeNames.SeverIs to avoid confusion), or (B) if it’s a typo, rename EdgeNames.SeverIs to EdgeNames.ServerIs and update SiteServer.SeverIs and the unit test assertions to use the corrected name, ensuring backward compatibility (add the old constant as an obsolete/deprecated alias if needed).test/unit/LdapPropertyTests.cs (1)
1089-1140: ⚡ Quick winAssert
domainsidis filtered out for site servers and subnets too.These two tests only guard
domainandname, whileReadSitePropertiesalso treatsdomainsidas a reserved/common field. A regression that leaksdomainsidfromReadSiteServerPropertiesorReadSiteSubnetPropertieswould currently pass.Suggested assertions
Assert.DoesNotContain("domain", keys); Assert.DoesNotContain("name", keys); + Assert.DoesNotContain("domainsid", keys); Assert.Contains("description", keys); Assert.Contains("whencreated", keys); @@ Assert.DoesNotContain("domain", keys); Assert.DoesNotContain("name", keys); + Assert.DoesNotContain("domainsid", keys); Assert.Contains("description", keys); Assert.Contains("whencreated", keys);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/unit/LdapPropertyTests.cs` around lines 1089 - 1140, The tests for ReadSiteServerProperties and ReadSiteSubnetProperties are missing an assertion that "domainsid" is filtered out like in ReadSiteProperties; update both tests (for functions ReadSiteServerProperties and ReadSiteSubnetProperties) to assert that the returned keys do not contain "domainsid" (same key casing used elsewhere, e.g., "domain" and "name") so any regression leaking domainsid will fail.test/unit/SiteProcessorTest.cs (1)
29-29: ⚡ Quick winUse shared LDAP property constants instead of a magic key.
"siteObject"should reference the canonical constant to avoid silent contract drift between tests and production code.Suggested update
- ["siteObject"] = siteObject + [LDAPProperties.SiteObject] = siteObject🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/unit/SiteProcessorTest.cs` at line 29, Replace the magic string key "siteObject" in SiteProcessorTest with the canonical shared LDAP property constant (import the shared constant, e.g. SharedLdapProperties.SiteObject or the project’s LdapConstants.SiteObject) so the test uses the same key as production; update the dictionary entry that currently reads ["siteObject"] = siteObject to use the constant (e.g. [LdapConstants.SiteObject] = siteObject) and add the appropriate using/import for the constants class.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/unit/SiteProcessorTest.cs`:
- Around line 158-160: The test and constants use a typo "SeverIs"; rename the
constant/property to "ServerIs" and update all references: change
EdgeNames.SeverIs to EdgeNames.ServerIs, change the SiteServer property
(SiteServer.SeverIs) to SiteServer.ServerIs, update the unit test
SiteServer_SeverIs_EdgeNameMatchesOutputProperty to assert
nameof(SiteServer.ServerIs) equals EdgeNames.ServerIs, and update any downstream
consumers or schema docs that reference the old name; if you must keep the
legacy name for compatibility, add a documented alias (deprecated SeverIs
forwarding to ServerIs) and update the test to reflect the deprecation behavior,
then run and verify unit tests.
---
Outside diff comments:
In `@src/CommonLib/Processors/SiteProcessor.cs`:
- Around line 86-100: The resolution methods currently return whatever
_utils.ResolveDistinguishedName yields without verifying the result is actually
a Site; update GetContainingSiteForServer and GetContainingSiteForSubnet to call
_utils.ResolveDistinguishedName as now, then validate the returned
TypedPrincipal represents a Site (e.g., check TypedPrincipal.Type or
ObjectClass/IsSite property on the returned Principal). If the resolved
principal is not a Site, return (false, default(TypedPrincipal)) (or the
existing failure tuple pattern) so callers don’t create containedBy edges to
non-site objects. Ensure both methods use the same validation logic after
calling _utils.ResolveDistinguishedName.
- Around line 139-151: The loop currently yields GPLink whenever
_utils.ResolveDistinguishedName(link.DistinguishedName) returns res.Success,
which can produce edges for non-GPOs; update the logic in the foreach over
Helpers.SplitGPLinkProperty(gpLink) to additionally verify the resolved
principal is a GPO (e.g., inspect res.Principal.ObjectClass or
res.Principal.Label for the GPO identifier used in your model) before yielding a
GPLink with res.Principal.ObjectIdentifier and IsEnforced; only emit the GPLink
when both res.Success is true and the principal indicates a GPO.
---
Nitpick comments:
In `@src/CommonLib/Enums/EdgeNames.cs`:
- Line 27: EdgeNames defines a constant EdgeNames.SeverIs ("SeverIs") that
matches SiteServer.SeverIs in tests, but this looks like a potential typo
vis-à-vis external BloodHound/SharpHound schemas; verify which literal the
external schema expects and then either (A) if the external contract truly uses
"SeverIs", add a clarifying comment and keep the constant as-is (or add an
explicit alias const string ServerIs = EdgeNames.SeverIs to avoid confusion), or
(B) if it’s a typo, rename EdgeNames.SeverIs to EdgeNames.ServerIs and update
SiteServer.SeverIs and the unit test assertions to use the corrected name,
ensuring backward compatibility (add the old constant as an obsolete/deprecated
alias if needed).
In `@test/unit/LdapPropertyTests.cs`:
- Around line 1089-1140: The tests for ReadSiteServerProperties and
ReadSiteSubnetProperties are missing an assertion that "domainsid" is filtered
out like in ReadSiteProperties; update both tests (for functions
ReadSiteServerProperties and ReadSiteSubnetProperties) to assert that the
returned keys do not contain "domainsid" (same key casing used elsewhere, e.g.,
"domain" and "name") so any regression leaking domainsid will fail.
In `@test/unit/SiteProcessorTest.cs`:
- Line 29: Replace the magic string key "siteObject" in SiteProcessorTest with
the canonical shared LDAP property constant (import the shared constant, e.g.
SharedLdapProperties.SiteObject or the project’s LdapConstants.SiteObject) so
the test uses the same key as production; update the dictionary entry that
currently reads ["siteObject"] = siteObject to use the constant (e.g.
[LdapConstants.SiteObject] = siteObject) and add the appropriate using/import
for the constants class.
🪄 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: 959ae3e4-08b3-47e3-ad28-4556bd56bac5
📒 Files selected for processing (10)
src/CommonLib/Enums/EdgeNames.cssrc/CommonLib/OutputTypes/SiteServer.cssrc/CommonLib/Processors/SiteProcessor.cstest/unit/ACLProcessorTest.cstest/unit/DirectoryObjectTests.cstest/unit/LDAPFilterTest.cstest/unit/LDAPUtilsTest.cstest/unit/LdapProducerQueryGeneratorTest.cstest/unit/LdapPropertyTests.cstest/unit/SiteProcessorTest.cs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/CommonLib/OutputTypes/SiteServer.cs
- Fix typo in ServerIs edge name - Fix collection scope - Fix node names
2 participants
Description
This pull requests aims to integrate Active Directory Sites into the data collected from the configuration partition.
This pull request goes with the following Bloodhound PR: SpecterOps/BloodHound#2031, as well as the following SharpHound PR: SpecterOps/SharpHound#186
A more complete description on why we thought this could be a good idea is described in details in the BloodHound PR, as well as the following article that we just released: https://www.synacktiv.com/en/publications/site-unseen-enumerating-and-attacking-active-directory-sites
Motivation and Context
Motivation and context are explained in the linked article.
How Has This Been Tested?
This has for the moment been tested in local lab instances. The lab instance was composed of various Active Directory site configurations:
Screenshots (if appropriate):
See the BloodHound PR and linked article for screenshots.
Types of changes
Checklist:
As was mentioned in the BloodHound pull request, this is of course only an implementation suggestion, and I remain open to discussion about the choices that were made, requests for implementation changes and so on!
Cheers,
Summary by CodeRabbit
New Features
Enhancements
Tests
Chores