Draft
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 22 minutes and 49 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughAdds extraction and filtering of “custom deny” ACEs from object security descriptors, configurable via a new Changes
Sequence DiagramsequenceDiagram
participant LP as LdapPropertyProcessor
participant ACLP as ACLProcessor
participant LU as LdapUtils
participant SD as RawSecurityDescriptor
LP->>ACLP: AddCustomDenyAcesProperty(props, ntSecurityDescriptor, objectDomain, label, distinguishedName, isMSA)
alt SkipDenyAces true or descriptor missing
ACLP-->>LP: return (no enrichment)
else Process deny ACEs
ACLP->>SD: Parse DiscretionaryAcl from descriptor
SD-->>ACLP: list of deny ACEs
loop for each deny ACE
ACLP->>LU: ResolveAccountName(trusteeSid)
LU-->>ACLP: resolvedName / domain info
ACLP->>ACLP: Apply filters (Exchange DN, Exchange trustee names per-domain cache, default-AD deny patterns, OU/container delete-tree exceptions)
alt ACE kept
ACLP->>ACLP: Rehydrate ACE into minimal ACL -> Serialize to SDDL fragment
end
end
ACLP->>LP: props["customdenyaces"] = [SDDL fragments]
end
LP-->>LP: Return enriched property dictionary
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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: 4
🧹 Nitpick comments (2)
src/CommonLib/LdapConfig.cs (1)
16-16: Log the new switch with the rest of the LDAP config.Line 16 adds a runtime collection switch, but
ToString()omits it even though LDAP config changes are logged. Including it makescustomdenyacessuppression easier to diagnose.Proposed observability tweak
sb.AppendLine($"AuthType: {AuthType.ToString()}"); sb.AppendLine($"MaxConcurrentQueries: {MaxConcurrentQueries}"); + sb.AppendLine($"SkipDenyAces: {SkipDenyAces}"); if (!string.IsNullOrWhiteSpace(Username)) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/CommonLib/LdapConfig.cs` at line 16, The ToString() for class LdapConfig is missing the new SkipDenyAces property so runtime logs don't show the custom deny-aces switch; update the LdapConfig.ToString() method to include the SkipDenyAces value (use the same naming convention used for other settings—e.g., "SkipDenyAces" or "customdenyaces") so when an LdapConfig instance is logged it prints the boolean state of SkipDenyAces along with the other properties.src/CommonLib/ILdapUtils.cs (1)
160-162: Prefer a narrow read-only accessor for the new flag.Line 162 exposes the full mutable LDAP config just so callers can read
SkipDenyAces. SinceLdapUtils.SetLdapConfig()rebuilds connection state, callers mutating the returned config could bypass those side effects. Prefer a narrower accessor or immutable snapshot.Example narrower API
- LdapConfig GetLdapConfig(); + bool ShouldSkipDenyAces();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/CommonLib/ILdapUtils.cs` around lines 160 - 162, The interface currently exposes a mutable LdapConfig via ILdapUtils.GetLdapConfig(), which lets callers mutate config and bypass SetLdapConfig() side effects; replace this with a narrow read-only accessor (for example add a bool SkipDenyAces { get; } or a GetSkipDenyAces() method) or return an immutable snapshot type (e.g., ReadOnlyLdapConfig) instead of LdapConfig, update callers to read the specific flag from the new accessor, and ensure SetLdapConfig() remains the only way to change runtime config/connection state.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/CommonLib/Processors/ACLProcessor.cs`:
- Around line 914-923: The try/catch around RawSecurityDescriptor construction
in ACLProcessor should also catch ArgumentException (not just OverflowException)
because RawSecurityDescriptor(byte[], int) can throw ArgumentException for
malformed self-relative SECURITY_DESCRIPTOR bytes; update the catch block that
wraps new RawSecurityDescriptor(ntSecurityDescriptor, 0) to handle both
OverflowException and ArgumentException (or add a separate catch for
ArgumentException) and log the same warning (including objectName) and return
Array.Empty<string>() so malformed descriptors from LDAP don't propagate an
unhandled exception.
- Around line 995-1011: The Everyone DeleteChild deny check is too narrow (only
Label.Domain); update the condition in ACLProcessor.cs inside the "Filter
default Everyone Deny ACEs" block so DeleteChild denies from Everyone are
treated as default accidental-deletion for container-like parents as well (e.g.,
Label.Domain OR Label.OU OR Label.Container) instead of only Label.Domain —
modify the objectType check that guards ActiveDirectoryRights.DeleteChild to
include those additional Label values and keep the existing return true behavior
so such default parent DeleteChild denies are filtered out.
In `@test/unit/LdapPropertyTests.cs`:
- Around line 154-157: Add a positive control run before the existing assertion:
call LdapPropertyProcessor.ReadOUProperties with the same mock/ldapUtils under
default configuration (i.e., without SkipDenyAces enabled) and assert that the
returned dictionary contains the "customdenyaces" key; then run the existing
test path that enables SkipDenyAces and assert that "customdenyaces" is not
present. Use the same mock variable and LdapPropertyProcessor instance creation
pattern so you verify the property is emitted normally and only suppressed when
SkipDenyAces is applied.
- Around line 139-143: The test method
LDAPPropertyProcessor_ReadOUProperties_SkipsCustomDenyAces_WhenLdapConfigRequestsIt
constructs a SecurityIdentifier (Windows-only API) but lacks platform gating;
annotate the test to run only on Windows by replacing or supplementing the
[Fact] attribute with the Windows-only attributes used elsewhere (e.g., add
[SupportedOSPlatform("windows")] and use [WindowsOnlyFact] or equivalent) so the
SecurityIdentifier construction and related assertions are skipped on
non-Windows platforms.
---
Nitpick comments:
In `@src/CommonLib/ILdapUtils.cs`:
- Around line 160-162: The interface currently exposes a mutable LdapConfig via
ILdapUtils.GetLdapConfig(), which lets callers mutate config and bypass
SetLdapConfig() side effects; replace this with a narrow read-only accessor (for
example add a bool SkipDenyAces { get; } or a GetSkipDenyAces() method) or
return an immutable snapshot type (e.g., ReadOnlyLdapConfig) instead of
LdapConfig, update callers to read the specific flag from the new accessor, and
ensure SetLdapConfig() remains the only way to change runtime config/connection
state.
In `@src/CommonLib/LdapConfig.cs`:
- Line 16: The ToString() for class LdapConfig is missing the new SkipDenyAces
property so runtime logs don't show the custom deny-aces switch; update the
LdapConfig.ToString() method to include the SkipDenyAces value (use the same
naming convention used for other settings—e.g., "SkipDenyAces" or
"customdenyaces") so when an LdapConfig instance is logged it prints the boolean
state of SkipDenyAces along with the other properties.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 258ec52f-a3e7-470f-b1ac-bcebba1bd691
📒 Files selected for processing (11)
docfx/Docfx.csprojsrc/CommonLib/Enums/DirectoryPaths.cssrc/CommonLib/ILdapUtils.cssrc/CommonLib/LdapConfig.cssrc/CommonLib/LdapUtils.cssrc/CommonLib/Processors/ACLProcessor.cssrc/CommonLib/Processors/LdapPropertyProcessor.cssrc/CommonLib/WellKnownPrincipal.cstest/unit/ACLProcessorTest.cstest/unit/Facades/MockLdapUtils.cstest/unit/LdapPropertyTests.cs
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
test/unit/LdapPropertyTests.cs (1)
139-143:⚠️ Potential issue | 🟠 MajorGate this test as Windows-only.
Line 142 constructs a
SecurityIdentifier, while similar tests in this file use[SupportedOSPlatform("windows")]and[WindowsOnlyFact]. This can still break non-Windows test runs or platform analyzer enforcement.🛡️ Proposed fix
- [Fact] + [SupportedOSPlatform("windows")] + [WindowsOnlyFact] public async Task LDAPPropertyProcessor_ReadOUProperties_SkipsCustomDenyAces_WhenLdapConfigRequestsIt()Run this read-only check to confirm remaining ungated
SecurityIdentifiertest usage:#!/bin/bash set -euo pipefail rg -n -C2 '\[Fact\]|\[Theory\]|\[WindowsOnlyFact\]|new SecurityIdentifier\(' --iglob '*.cs' test/unit rg -n '<TreatWarningsAsErrors|EnableNETAnalyzers|AnalysisLevel|SupportedOSPlatform' --iglob '*.csproj' --iglob '*.props' --iglob '*.targets'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/LdapPropertyTests.cs` around lines 139 - 143, The test method LDAPPropertyProcessor_ReadOUProperties_SkipsCustomDenyAces_WhenLdapConfigRequestsIt creates a SecurityIdentifier which is Windows-only; mark this test with the appropriate platform attributes (e.g., add [SupportedOSPlatform("windows")] and replace [Fact] with [WindowsOnlyFact] or add [WindowsOnlyFact] alongside [Fact] per project conventions) so the SecurityIdentifier construction is gated on Windows; update the method declaration attributes accordingly to match other tests in this file (refer to the method name and the SecurityIdentifier instantiation) to prevent non-Windows runs or analyzer failures.src/CommonLib/Processors/ACLProcessor.cs (1)
995-1011:⚠️ Potential issue | 🟠 MajorTighten the default Everyone deny filter.
These
HasFlagchecks suppress ACEs that contain the default bits plus additional custom deny rights, socustomdenyacescan silently miss relevant ACEs. Also, theDeleteChilddefault still only filters domains, leaving accidental-deletion parent ACEs on OUs/containers to be emitted.🛡️ Proposed fix
// Filter default Everyone Deny ACEs if (principalSid.Equals(WellKnownPrincipal.EveryoneSid, StringComparison.OrdinalIgnoreCase)) { if ((objectType is Label.OU or Label.Container) && - rights.HasFlag(ActiveDirectoryRights.Delete) && - rights.HasFlag(ActiveDirectoryRights.DeleteTree)) { + HasOnlyRights(rights, ActiveDirectoryRights.Delete | ActiveDirectoryRights.DeleteTree)) { return true; } if (isMSA && - rights.HasFlag(ActiveDirectoryRights.ExtendedRight) && + HasOnlyRights(rights, ActiveDirectoryRights.ExtendedRight) && objectAceType.Equals(new Guid(ACEGuids.UserForceChangePassword))) { return true; } - if (objectType == Label.Domain && rights.HasFlag(ActiveDirectoryRights.DeleteChild)) { + if ((objectType is Label.Domain or Label.OU or Label.Container or Label.Configuration) && + HasOnlyRights(rights, ActiveDirectoryRights.DeleteChild)) { return true; } }Add this helper near the filtering helpers:
private static bool HasOnlyRights(ActiveDirectoryRights actual, ActiveDirectoryRights expected) { return (actual & expected) == expected && (actual & ~expected) == 0; }Based on learnings, properties added to dictionaries returned by methods in SharpHoundCommon may be consumed by dependent projects like SharpHound and SharpHoundEnterprise.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/CommonLib/Processors/ACLProcessor.cs` around lines 995 - 1011, The Everyone-deny filtering in ACLProcessor (the block that checks principalSid == WellKnownPrincipal.EveryoneSid and inspects objectType, rights, isMSA, and objectAceType) currently uses HasFlag which allows extra custom deny bits to be suppressed; add the proposed helper HasOnlyRights(ActiveDirectoryRights actual, ActiveDirectoryRights expected) and replace the HasFlag checks with calls to HasOnlyRights for the Delete/DeleteTree, ExtendedRight/UserForceChangePassword, and DeleteChild checks so only ACEs that match exactly the default deny bits are filtered; also broaden the DeleteChild default filter so it applies to OU and Container objectType values (not just Domain) to prevent accidental suppression of parent-deletion ACEs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/CommonLib/Processors/LdapPropertyProcessor.cs`:
- Around line 670-683: The AddCustomDenyAceProperty method is passing the
distinguished name into both the 5th and 7th parameters of
_aclProcessor.AddCustomDenyAcesProperty (distinguishedName and objectName);
change the 7th argument to pass the object's actual name (e.g., sAMAccountName
retrieved via entry.TryGetStringProperty("sAMAccountName", out var objectName)
or the entry.TryGetSamAccountName helper if available) instead of using
distinguishedName, and handle the TryGetDistinguishedName return (use a safe
default like string.Empty if it returns false) so the 5th parameter never
unexpectedly becomes null. Ensure you update the call site in
AddCustomDenyAceProperty to pass the fetched objectName variable and not
distinguishedName.
---
Duplicate comments:
In `@src/CommonLib/Processors/ACLProcessor.cs`:
- Around line 995-1011: The Everyone-deny filtering in ACLProcessor (the block
that checks principalSid == WellKnownPrincipal.EveryoneSid and inspects
objectType, rights, isMSA, and objectAceType) currently uses HasFlag which
allows extra custom deny bits to be suppressed; add the proposed helper
HasOnlyRights(ActiveDirectoryRights actual, ActiveDirectoryRights expected) and
replace the HasFlag checks with calls to HasOnlyRights for the
Delete/DeleteTree, ExtendedRight/UserForceChangePassword, and DeleteChild checks
so only ACEs that match exactly the default deny bits are filtered; also broaden
the DeleteChild default filter so it applies to OU and Container objectType
values (not just Domain) to prevent accidental suppression of parent-deletion
ACEs.
In `@test/unit/LdapPropertyTests.cs`:
- Around line 139-143: The test method
LDAPPropertyProcessor_ReadOUProperties_SkipsCustomDenyAces_WhenLdapConfigRequestsIt
creates a SecurityIdentifier which is Windows-only; mark this test with the
appropriate platform attributes (e.g., add [SupportedOSPlatform("windows")] and
replace [Fact] with [WindowsOnlyFact] or add [WindowsOnlyFact] alongside [Fact]
per project conventions) so the SecurityIdentifier construction is gated on
Windows; update the method declaration attributes accordingly to match other
tests in this file (refer to the method name and the SecurityIdentifier
instantiation) to prevent non-Windows runs or analyzer failures.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0c6a129b-8e11-46d8-a9b6-54653652f8bf
📒 Files selected for processing (7)
src/CommonLib/ILdapUtils.cssrc/CommonLib/LdapConfig.cssrc/CommonLib/LdapUtils.cssrc/CommonLib/Processors/ACLProcessor.cssrc/CommonLib/Processors/LdapPropertyProcessor.cstest/unit/Facades/MockLdapUtils.cstest/unit/LdapPropertyTests.cs
✅ Files skipped from review due to trivial changes (1)
- src/CommonLib/LdapConfig.cs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/CommonLib/ILdapUtils.cs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Adds support for a new BloodHound node property,
customdenyaces, which captures filtered deny ACEs from an AD object'snTSecurityDescriptoras individual SDDL strings. The property is emitted for ACL-backed AD objects only when qualifying deny ACEs remain after filtering.This change also adds a shared
LdapConfig.SkipDenyAcesswitch so SharpHound can disable collection of this property if it causes unexpected issues.The change also disables DocFX builds by default on non-Windows environments to avoid unnecessary cross-platform build issues.
Motivation and Context
This adds auditing-only visibility into custom deny ACEs (no edge impact). It helps surface potentially interesting deny entries while excluding known noisy/default patterns such as Exchange-related denies, accidental deletion protection, and a small set of default AD deny ACEs.
This PR addresses: BED-8117
This PR relates to the following PRs:
How Has This Been Tested?
Added and updated unit tests in
test/unitcovering:SkipDenyAcessuppresses emission ofcustomdenyacesI also updated existing property-reader tests affected by the
LdapPropertyProcessorchanges.Screenshots (if appropriate):
N/A
Types of changes
Checklist:
Summary by CodeRabbit
New Features
Chores
Tests