Feature: Add collection of Active Directory Sites ACLs and data #186
Conversation
* Process data associated with sites, sites servers, sites subnets * Add collection method "site", included in default collection methods
WalkthroughActive Directory Site objects (Sites, SiteServers, SiteSubnets) are now supported as a collection method. Changes include enum registration, command-line option parsing, runtime object processing with ACL handling and containment logic, and corresponding output writers. Changes
Sequence DiagramsequenceDiagram
participant CLI as CLI/Options
participant Proc as ObjectProcessors
participant ACL as ACL Handler
participant Writer as OutputWriter
CLI->>CLI: Parse Site collection method
CLI->>Proc: Route Site/SiteServer/SiteSubnet objects
rect rgb(200, 230, 255)
Note over Proc,ACL: Object Processing Phase
Proc->>ACL: Read ACLs
Proc->>Proc: Merge object properties
Proc->>Proc: Deduce containment relationships
end
Proc->>Writer: Pass processed objects
rect rgb(230, 200, 255)
Note over Writer: Output Phase
Writer->>Writer: Write Site objects
Writer->>Writer: Write SiteServer objects
Writer->>Writer: Write SiteSubnet objects
end
Writer->>Writer: Flush writers & create archive
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–25 minutes
Possibly related issues
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 (1)
src/Runtime/OutputWriter.cs (1)
219-220: Minor formatting: missing space after comma.Line 219 is missing a space between
_certTemplateOutput.GetFilename(),and_issuancePolicyOutput.GetFilename().Apply this diff:
- _certTemplateOutput.GetFilename(),_issuancePolicyOutput.GetFilename(), + _certTemplateOutput.GetFilename(), _issuancePolicyOutput.GetFilename(),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/Client/Enums.cs(1 hunks)src/Options.cs(2 hunks)src/Runtime/ObjectProcessors.cs(4 hunks)src/Runtime/OutputWriter.cs(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/Runtime/ObjectProcessors.cs (2)
src/Runtime/OutputWriter.cs (2)
Task(108-170)Task(172-193)src/Client/Context.cs (2)
Task(72-72)Dictionary(23-27)
src/Runtime/OutputWriter.cs (1)
src/Writers/JsonDataWriter.cs (3)
JsonDataWriter(19-119)JsonDataWriter(33-47)GetFilename(115-118)
🔇 Additional comments (7)
src/Options.cs (1)
17-17: LGTM! Site collection method properly integrated.The Site option is correctly added to the help text and mapped in the switch statement, consistent with existing collection methods.
Also applies to: 211-211
src/Client/Enums.cs (1)
34-34: LGTM! Enum member correctly positioned.The Site enum member is properly placed and aligns with the mapping in Options.cs.
src/Runtime/OutputWriter.cs (1)
38-40: LGTM! Site-related output writers properly integrated.The three new writers (Site, SiteServer, SiteSubnet) follow the established pattern consistently across field declarations, constructor initialization, routing in StartWriter, and flushing.
Also applies to: 64-66, 155-163, 187-189
src/Runtime/ObjectProcessors.cs (4)
42-42: LGTM! SiteProcessor properly initialized.The SiteProcessor field and constructor initialization follow the established pattern used by other processors.
Also applies to: 64-64
100-105: LGTM! Switch cases properly route Site-related labels.The three new case statements correctly dispatch Site, SiteServer, and SiteSubnet objects to their respective processing methods.
1049-1052: Verify: ContainedBy logic nested under ObjectProps rather than Container flag.The containing site for subnets is determined inside the
ObjectPropsconditional block (lines 1037-1053) rather than aContainercheck. While the comment on lines 1047-1048 explains this requires reading thesiteObjectproperty, this creates an inconsistency: theContainedByfield won't be populated ifObjectPropsisn't enabled, even whenContainercollection is active.Consider whether the site lookup could be moved under a
Containerflag check, or ifObjectPropsshould implicitly enable the required property reads for containment.
970-970: Verify: GP link reading inconsistency in ProcessSiteObject requires developer confirmationThe inconsistency identified in the review is confirmed. ProcessDomainObject (line 530) and ProcessOUObject (line 603) both guard GP link reading with
if (_methods.HasFlag(CollectionMethod.Container)), but ProcessSiteObject (line 970) reads GP links unconditionally without checking the Container flag.Other conditional properties in ProcessSiteObject (ACL, ObjectProps) follow the flag-based pattern, making this inconsistency stand out. No comments explain why Site differs.
This requires developer verification: is the unconditional GP link reading for Site intentional (perhaps Site objects always require links regardless of collection settings), or is it a bug that should follow the Domain/OU pattern?
| if (await _siteProcessor.GetContainingSiteForServer(entry) is (true, var container)) | ||
| { | ||
| ret.ContainedBy = container; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Verify: Should containing site lookup be conditional on Container collection method?
Lines 986-989 get the containing site for the server unconditionally. Other similar methods only retrieve containment relationships when the Container collection method is enabled:
ProcessUserObject(lines 205-212) checksContainerflagProcessComputerObject(lines 274-279) checksContainerflagProcessGroupObject(lines 482-487) checksContainerflag
Run the following script to examine containment patterns:
🏁 Script executed:
#!/bin/bash
# Search for ContainedBy assignment patterns to verify conditional checks
rg -nP -B5 'ret\.ContainedBy\s*=' --type=cs | rg -A5 -B5 'ProcessObject|HasFlag.*Container'Length of output: 6055
🏁 Script executed:
#!/bin/bash
# View ProcessSiteServerObject method around lines 975-1005 to verify Container check presence
sed -n '975,1005p' src/Runtime/ObjectProcessors.cs | cat -nLength of output: 74
🏁 Script executed:
#!/bin/bash
# View ProcessSiteServerObject method around lines 975-1005
sed -n '975,1005p' src/Runtime/ObjectProcessors.cs | cat -nLength of output: 1717
🏁 Script executed:
#!/bin/bash
# View ProcessSiteSubnetObject method around lines 1016-1056 to verify its containment pattern
sed -n '1016,1056p' src/Runtime/ObjectProcessors.cs | cat -nLength of output: 2208
Wrap containing site lookups in Container collection method checks
Verification confirms the issue. All 12 other object processors guard ContainedBy assignments with HasFlag(CollectionMethod.Container) checks, but:
- ProcessSiteServerObject (lines 986-989): Retrieves containing site unconditionally—needs
Containerflag guard - ProcessSiteSubnetObject (lines 1049-1052): Retrieves containing site inside the
ObjectPropsblock instead of aContainerblock—should be conditional onContainerflag (the comment about needingsiteObjectproperty explains data dependencies, but architectural responsibility for containment should follow Container flag, not ObjectProps)
Both deviations break consistency with ProcessUserObject, ProcessComputerObject, ProcessGroupObject, and all domain/certificate processors.
🤖 Prompt for AI Agents
In src/Runtime/ObjectProcessors.cs around lines 986-989 and 1049-1052, the code
retrieves the containing site unconditionally (lines 986-989) and inside the
ObjectProps block (lines 1049-1052); wrap these containing-site lookups and the
assignments to ret.ContainedBy with a guard that checks
entry.Methods.HasFlag(CollectionMethod.Container) (i.e., only perform the
_siteProcessor.GetContainingSiteForServer / GetContainingSiteForSubnet calls and
set ret.ContainedBy when the Container flag is present), and move the subnet
lookup out of the ObjectProps block into the Container-guarded section so
containment logic matches the other processors.
1 participant
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
It is also accompanied by the following SharpHoundCommon PR: SpecterOps/SharpHoundCommon#257
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