Skip to content

Conversation

JonasBK
Copy link
Contributor

@JonasBK JonasBK commented Sep 3, 2025

Describe collection of additional DC reg key.

Corresponding PRs:
Add netlogon reg key by JonasBK · Pull Request #244 · SpecterOps/SharpHoundCommon
Add netlogon reg key by JonasBK · Pull Request #61 · SpecterOps/sharphound-enterprise
Add netlogon reg key by JonasBK · Pull Request #170 · SpecterOps/SharpHound
Add netlogon DC property by JonasBK · Pull Request #1845 · SpecterOps/BloodHound

Also fixed a couple of related things:

  • Add RoleSeparationEnabled collection under CA Registry
  • Fixed formatting issues under CA Registry

Screenshots:
image

Summary by CodeRabbit

  • Documentation

    • Improved DC registry section: inline code formatting for key names; added Enterprise-specific VulnerableChannelAllowList under Netlogon parameters; refined spacing and links.
    • Refreshed CA registry section: code-formatted path for CertSvc configuration; restructured keys into explicit entries (EnrollmentAgentRights, Security, PolicyModules<Active Policy>\EditFlags); added RoleSeparationEnabled; standardized labels and formatting.
    • Standardized code formatting for paths/keys and adjusted spacing for readability.
  • Chores

    • General formatting tidy-up across the affected documentation pages.

Copy link
Contributor

coderabbitai bot commented Sep 3, 2025

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Walkthrough

Edits to docs/collect-data/permissions.mdx standardize registry path formatting, restructure DC and CA registry sections, add specific keys (e.g., VulnerableChannelAllowList, RoleSeparationEnabled), switch EditFlags to use an Active Policy placeholder path, and make minor spacing/link formatting adjustments.

Changes

Cohort / File(s) Summary
DC Registry formatting and keys
docs/collect-data/permissions.mdx
Inline-code formatting for registry keys (Kdc\StrongCertificateBindingEnforcement, Schannel\CertificateMappingMethods). Added SYSTEM\CurrentControlSet\Services\Netlogon\Parameters\VulnerableChannelAllowList. Inserted spacing and updated links.
CA Registry structure and keys
docs/collect-data/permissions.mdx
Reformatted CA path to SYSTEM\CurrentControlSet\Services\CertSvc\Configuration\<CA Name>. Clarified entries for EnrollmentAgentRights, Security, PolicyModules\<Active Policy>\EditFlags (dynamic path), and added RoleSeparationEnabled. Consistent labeling.
Global formatting tidy-ups
docs/collect-data/permissions.mdx
Standardized code formatting for paths/keys and minor spacing/readability updates.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

Suggested labels

documentation

Suggested reviewers

  • psionicjake

Poem

Thump-thump go my paws, neat paths in a row,
Keys in backticks gleam with a tidy glow.
DC hums, CA nods—flags all aligned,
Policies point where they’re properly defined.
A hop, a wink, and a docs page new—
Carrots for clarity! 🥕✨

Pre-merge checks (3 passed)

✅ Passed checks (3 passed)
Check name Status Explanation
Title Check ✅ Passed The title “Update registry collection methods” directly reflects the primary purpose of the changeset—modifying how registry keys are documented for collection—and does so in a concise and focused manner without unnecessary detail.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.
Description Check ✅ Passed The pull request description accurately summarizes the addition of the new DC registry key, references the related code PRs, and notes the CA Registry fixes and formatting updates shown in the changeset. It clearly relates to the documented modifications and provides context through linked PRs and screenshots.
✨ Finishing Touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch sh-collection-permissions

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.

@StephenHinck
Copy link
Contributor

@coderabbitai review

Copy link
Contributor

coderabbitai bot commented Sep 8, 2025

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Copy link
Contributor

@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: 1

🧹 Nitpick comments (1)
docs/collect-data/permissions.mdx (1)

108-110: Tighten wording for accuracy and flow.

Minor phrasing tweaks improve clarity; no change in meaning.

-SharpHound collects the registry key values `Kdc\StrongCertificateBindingEnforcement` and `Schannel\CertificateMappingMethods` (described [here](https://support.microsoft.com/en-us/topic/kb5014754-certificate-based-authentication-changes-on-windows-domain-controllers-ad2c23b0-15d8-4340-a468-4d4f3b188f16)) to determine the allowed certificate mapping methods by the DCs. The BloodHound ADCS edges ESC6, ESC9, and ESC10 require this data to be collected.
+SharpHound collects the registry values `Kdc\StrongCertificateBindingEnforcement` and `Schannel\CertificateMappingMethods` (described [here](https://support.microsoft.com/en-us/topic/kb5014754-certificate-based-authentication-changes-on-windows-domain-controllers-ad2c23b0-15d8-4340-a468-4d4f3b188f16)) to determine the allowed certificate mapping methods on domain controllers (DCs). The BloodHound ADCS edges ESC6, ESC9, and ESC10 require this data to be collected.
 
-SharpHound Enterprise additionally collects the `VulnerableChannelAllowList` value under `SYSTEM\CurrentControlSet\Services\Netlogon\Parameters` (described [here](https://support.microsoft.com/en-us/topic/how-to-manage-the-changes-in-netlogon-secure-channel-connections-associated-with-cve-2020-1472-f7e8cc17-0309-1d6a-304e-5ba73cd1a11e#theGroupPolicy)) to determine accounts allowed Netlogon secure channel without secure RPC. 
+SharpHound Enterprise additionally collects the `VulnerableChannelAllowList` value under `SYSTEM\CurrentControlSet\Services\Netlogon\Parameters` (described [here](https://support.microsoft.com/en-us/topic/how-to-manage-the-changes-in-netlogon-secure-channel-connections-associated-with-cve-2020-1472-f7e8cc17-0309-1d6a-304e-5ba73cd1a11e#theGroupPolicy)) to determine which accounts are allowed to use Netlogon secure channel connections without secure RPC.
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ffa71a6 and cb273bb.

📒 Files selected for processing (1)
  • docs/collect-data/permissions.mdx (1 hunks)

@StephenHinck StephenHinck added the Waiting for BH PR Don't merge this doc update until the related BH feature is merged and released. label Sep 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Waiting for BH PR Don't merge this doc update until the related BH feature is merged and released.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants