Feature: Add collection of Active Directory Sites ACLs and data #257
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
WalkthroughThis PR extends the BloodHound data collection platform with comprehensive support for Active Directory Sites, SiteServers, and SiteSubnets. The changes introduce new enums, LDAP query filters, output types, and processors to collect and handle site-related objects and their relationships, including GPLink associations. Changes
Sequence DiagramsequenceDiagram
participant Collector
participant LdapProducerQueryGenerator
participant LdapFilter
participant LdapUtils
participant SiteProcessor
participant OutputTypes
Collector->>LdapProducerQueryGenerator: GenerateConfigurationPartitionParameters(with Site flag)
activate LdapProducerQueryGenerator
LdapProducerQueryGenerator->>LdapFilter: AddSites()
LdapProducerQueryGenerator->>LdapFilter: AddSiteServers()
LdapProducerQueryGenerator->>LdapFilter: AddSiteSubnets()
LdapProducerQueryGenerator->>Collector: Return queries with site filters
deactivate LdapProducerQueryGenerator
Collector->>LdapUtils: Query LDAP for site objects
activate LdapUtils
LdapUtils->>LdapUtils: ResolveLabel (classify objectClass as Site/SiteServer/SiteSubnet)
LdapUtils->>LdapUtils: ComputeDisplayName (format display name per type)
LdapUtils->>Collector: Return resolved search results
deactivate LdapUtils
Collector->>SiteProcessor: Process site results
activate SiteProcessor
SiteProcessor->>SiteProcessor: GetContainingSiteForServer/Subnet (resolve DN relationships)
SiteProcessor->>SiteProcessor: ReadSiteGPLinks (extract and parse GPLink entries)
SiteProcessor->>Collector: Return site relationships & GPLinks
deactivate SiteProcessor
Collector->>OutputTypes: Populate Site/SiteServer/SiteSubnet objects
activate OutputTypes
OutputTypes->>Collector: Return typed output
deactivate OutputTypes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related issues
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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.
| if (subnetProperties.TryGetValue("siteObject", out var siteObject)) | ||
| { | ||
| return await GetContainingSiteForSubnet(siteObject.ToString()); | ||
| } |
There was a problem hiding this comment.
Guard against null siteObject before ToString().
If LDAP omitted siteObject, ReadSiteSubnetProperties still adds the key with a null value. TryGetValue will succeed, and siteObject.ToString() will throw, killing the site subnet collection path. Please bail out when the value is missing before converting to string, e.g.:
- if (subnetProperties.TryGetValue("siteObject", out var siteObject))
- {
- return await GetContainingSiteForSubnet(siteObject.ToString());
- }
+ if (subnetProperties.TryGetValue("siteObject", out var siteObject)
+ && siteObject is string siteDn
+ && !string.IsNullOrWhiteSpace(siteDn))
+ {
+ return await GetContainingSiteForSubnet(siteDn);
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (subnetProperties.TryGetValue("siteObject", out var siteObject)) | |
| { | |
| return await GetContainingSiteForSubnet(siteObject.ToString()); | |
| } | |
| if (subnetProperties.TryGetValue("siteObject", out var siteObject) | |
| && siteObject is string siteDn | |
| && !string.IsNullOrWhiteSpace(siteDn)) | |
| { | |
| return await GetContainingSiteForSubnet(siteDn); | |
| } |
🤖 Prompt for AI Agents
In src/CommonLib/Processors/SiteProcessor.cs around lines 44 to 47, the code
calls siteObject.ToString() without guarding for a null value; if the LDAP entry
added the key with a null value TryGetValue will succeed and ToString() will
throw. Fix by checking the retrieved value for null (and optionally for
empty/whitespace) before converting: if it's null/empty, bail out the method
path (e.g., return null or skip processing) instead of calling ToString(),
otherwise call GetContainingSiteForSubnet with the safe string value.
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, 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