Skip to content

Conversation

@ncha-syn
Copy link

@ncha-syn ncha-syn commented Oct 24, 2025

Description

This pull request adds the ability to SharpHound to collect user privileges using GPOs and increases of the scope of the collected privileges.

Motivation and Context

This PR follows SpecterOps/BloodHound#1999.

How Has This Been Tested?

The changes have been tested manually on a lab environment.

Screenshots (if appropriate):

image

Types of changes

  • Chore (a change that does not modify the application functionality)
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • Documentation updates are needed, and have been made accordingly.
  • I have added and/or updated tests to cover my changes.
  • All new and existing tests passed.
  • My changes include a database migration.

Summary by CodeRabbit

  • New Features
    • Added GPOUserRights as a new collection method option
    • Available in command-line interface help text and PowerShell parameters
    • Updated documentation and configurations to support the new method
    • Extends available collection methods: Container, Group, LocalGroup, GPOLocalGroup, and GPOUserRights

@coderabbitai
Copy link

coderabbitai bot commented Oct 24, 2025

Walkthrough

This PR adds support for a new collection method called GPOUserRights. The changes include updating the enum definition, CLI help text, PowerShell parameter validation, and runtime processing logic to handle this new collection method, with specific processing for OU objects.

Changes

Cohort / File(s) Change Summary
Enum Definition
src/Client/Enums.cs
Added new enum member GPOUserRights to CollectionMethodOptions, inserted after GPOLocalGroup.
Configuration & Help Text
README.md, src/Options.cs
Updated CLI help text and collection methods option list to include GPOUserRights, and added mapping from CollectionMethodOptions.GPOUserRights to CollectionMethod.GPOUserRights in the resolution switch.
PowerShell Module
src/PowerShell/Template.ps1
Extended CollectionMethods parameter allowed values in Invoke-BloodHound to include GPOUserRights.
Runtime Processing
src/Runtime/ObjectProcessors.cs
Added GPOUserRightsAssignmentProcessor field and initialization in constructor; introduced processing hook for OU objects to fetch and assign GPO user rights via the new processor.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Suggested labels

enhancement, blocked by SHC PR

Suggested reviewers

  • ktstrader
  • definitelynotagoblin

Poem

🐰 A new collection method hops into view,
GPOUserRights now part of the queue,
From enums to PowerShell, the changes align,
Runtime processors handle OU's design—
BloodHound's reach grows ever so fine! 🎯

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Title Check ✅ Passed The PR title "Add local privilege collection" is related to the main change in the pull request. The changeset adds GPOUserRights as a new collection method across multiple components (enums, CLI options, PowerShell templates, and object processors), enabling SharpHound to collect user privileges via GPOs. The title appropriately summarizes this primary change, though it could be more specific about GPO-based collection. The title is clear and avoids vague terminology, making it sufficiently descriptive for scanning commit history.
Description Check ✅ Passed The PR description follows the required template structure with all major sections addressed. The Description section clearly states the purpose, the Motivation and Context section provides a link to a related BloodHound PR for context, the testing section indicates manual lab environment testing, and a screenshot is included. The author appropriately selected "New feature" for the type of change. While the testing section is relatively brief and lacks extensive detail, it is sufficiently filled out for evaluation, and the unchecked checklist items appear to be reasonable author judgment rather than oversights, as documentation updates, formal tests, and database migrations may not apply to this feature addition.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
src/Runtime/ObjectProcessors.cs (1)

365-365: Remove commented-out code.

These commented-out code blocks should be removed to maintain code cleanliness. If these represent alternative implementations being evaluated, consider using feature flags or creating a separate development branch.

Apply this diff to remove the commented-out code:

             if (_methods.HasFlag(CollectionMethod.SmbInfo)) {
                 ret.SmbInfo = await _smbProcessor.Scan(apiName, resolvedSearchResult.DomainSid);
-                //ret.SmbInfo = await _smbProcessor.Scan(apiName);
             }
                 if (ldapServices.IsSigningRequired.Collected) {
                     ret.Properties.Add("ldapsigning", ldapServices.IsSigningRequired.Result);
                 }
-                //var ldapServices = await dcLdapProcessor.Scan(resolvedSearchResult.DisplayName);
-                //ret.Properties.Add("ldapavailable", ldapServices.HasLdap);
-                //ret.Properties.Add("ldapsavailable", ldapServices.HasLdaps);
-                //if (ldapServices.IsChannelBindingDisabled.Collected) {
-                //    ret.Properties.Add("ldapsepa", !ldapServices.IsChannelBindingDisabled.Result);    
-                //}
-
-                //if (ldapServices.IsSigningRequired.Collected) {
-                //    ret.Properties.Add("ldapsigning", ldapServices.IsSigningRequired.Result);    
-                //}
             }

Also applies to: 431-440

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between dc420a6 and e4c0d99.

📒 Files selected for processing (5)
  • README.md (1 hunks)
  • src/Client/Enums.cs (1 hunks)
  • src/Options.cs (2 hunks)
  • src/PowerShell/Template.ps1 (1 hunks)
  • src/Runtime/ObjectProcessors.cs (6 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/Options.cs (3)
src/BaseContext.cs (1)
  • CollectionMethod (95-102)
src/Client/Context.cs (1)
  • CollectionMethod (75-75)
src/Extensions.cs (1)
  • CollectionMethod (114-119)
🔇 Additional comments (7)
src/Client/Enums.cs (1)

22-22: LGTM! Enum member positioned logically.

The new GPOUserRights enum member is appropriately placed alongside other GPO-related collection methods.

README.md (1)

38-38: LGTM! Documentation updated correctly.

The CLI help text accurately reflects the new collection method.

src/PowerShell/Template.ps1 (1)

30-30: LGTM! PowerShell parameter documentation updated.

The description clearly explains the new collection method's purpose.

src/Options.cs (2)

17-17: LGTM! Help text includes new collection method.

The help text correctly lists GPOUserRights among available collection methods.


199-199: Mapping is correct; composite method verification is external to this file.

The enum mapping at line 199 correctly maps CollectionMethodOptions.GPOUserRights to CollectionMethod.GPOUserRights, consistent with all other option mappings in the switch expression.

However, verification of whether GPOUserRights is appropriately included in composite methods (Default, All, DCOnly) is not applicable here. Those composite values are defined in the external SharpHoundCommonLib.Enums.CollectionMethod enum, not in this file. The code in Options.cs simply maps individual user-specified options to their corresponding enum values using bitwise OR operations—it does not construct or define composite methods.

The mapping itself contains no issues: it handles the new GPOUserRights option consistently with existing collection methods.

src/Runtime/ObjectProcessors.cs (2)

35-35: LGTM! Processor field and initialization follow established pattern.

The GPOUserRightsAssignmentProcessor is correctly declared and initialized, mirroring the approach used for GPOLocalGroupProcessor.

Also applies to: 60-60


615-617: Verify OU class has GPOUserRights property.

The implementation correctly processes GPO user rights for OUs when the collection method is enabled. Ensure that the OU class (in SharpHoundCommonLib.OutputTypes) has a corresponding GPOUserRights property defined to receive this data.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant