Feature: Add collection of Active Directory Sites ACLs and data#186
Feature: Add collection of Active Directory Sites ACLs and data#186q-roland wants to merge 6 commits into
Conversation
* Process data associated with sites, sites servers, sites subnets * Add collection method "site", included in default collection methods
|
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughActive Directory Site objects (Sites, SiteServers, SiteSubnets) are added as a collection method through command-line option parsing, runtime object processing with ACL handling and containment deduction, output serialization, and documentation updates. ChangesSite Collection Method Integration
Sequence DiagramsequenceDiagram
participant Parser as Options Parser
participant ObjProc as ObjectProcessors
participant SiteProc as SiteProcessor
participant Handler as Site Handlers
participant Writer as OutputWriter
Parser->>ObjProc: Route Site/SiteServer/SiteSubnet objects
rect rgba(100, 150, 200, 0.5)
Note over ObjProc,Handler: Object Processing Phase
ObjProc->>Handler: Dispatch to handler (Site/SiteServer/SiteSubnet)
Handler->>Handler: Populate common properties
Handler->>Handler: Process ACLs if enabled
Handler->>Handler: Merge LDAP properties if enabled
end
rect rgba(150, 100, 200, 0.5)
Note over Handler,SiteProc: Relationship Resolution
Handler->>SiteProc: Get containing site/referenced computer/subnet containment
SiteProc-->>Handler: Return relationship data
Handler->>Handler: Set ContainedBy and relationship fields
end
Handler->>Handler: Read GP links if Site + collection enabled
Handler->>Writer: Accept processed object
rect rgba(200, 150, 100, 0.5)
Note over Writer: Serialization Phase
Writer->>Writer: Write to JsonDataWriter
Writer->>Writer: Flush writers on completion
Writer->>Writer: Archive output files
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 (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?
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
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
Summary
New Features
--collectionmethodsoption.Documentation
--collectionmethodsto list Site.Invoke-BloodHoundhelp for collection-method presets, including the Site entry.