Revert "feat: AdminSDHolder updates - BED-6221" #173
Conversation
WalkthroughRemoves AdminSDHolder-related properties and processing across context and runtime, refactors LDAP producer to partition queries by objectGuid and adjusts Configuration NC search behavior, adds logging and error handling, and applies a non-functional newline to the project file. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller
participant LdapProducer
participant Context
participant LDAP
Note over LdapProducer: New partitioned query flow
Caller->>LdapProducer: Produce(...)
LdapProducer->>Context: Read Flags.PartitionLdapQueries
alt Partition enabled
LdapProducer->>LdapProducer: GetPartitionedFilter(original)
loop For each partitioned filter
LdapProducer->>LDAP: Search(filter_i)
alt Success
LdapProducer->>LdapProducer: Filter out unwanted entries
LdapProducer-->>Caller: Write entries to channel
else Error
LdapProducer->>LdapProducer: Log error and break inner loop
end
end
else Partition disabled
LdapProducer->>LDAP: Search(original filter)
LdapProducer-->>Caller: Write entries to channel
end
Note over LdapProducer: AdminSDHolder collection removed
sequenceDiagram
autonumber
participant Producer as LdapProducer
participant Context
participant LDAP
Note over Producer: Configuration NC search behavior
Producer->>Context: Try get Configuration NC path
alt Path found
Producer->>LDAP: SearchBase = path (ACLs respected)
else Path not found
Producer->>LDAP: NamingContext = Configuration (ACL handling preserved)
end
Producer-->>Producer: Log "Beginning LDAP search for {Domain} Configuration NC"
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests
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: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Runtime/ObjectProcessors.cs (1)
360-386: Avoid async void — convert to Task and await callersasync void swallows exceptions and allows concurrent mutation of shared state (e.g., ret). Convert these to Task and await call sites.
- private async void ProcessDomainController(ResolvedSearchResult resolvedSearchResult, Computer ret, - string apiName) { + private async Task ProcessDomainController(ResolvedSearchResult resolvedSearchResult, Computer ret, + string apiName) { @@ - if (resolvedSearchResult.IsDomainController) - ProcessDomainController(resolvedSearchResult, ret, apiName); + if (resolvedSearchResult.IsDomainController) + await ProcessDomainController(resolvedSearchResult, ret, apiName);
- Also fix src/Producers/StealthProducer.cs:94 — change
private async void BuildStealthTargets()toprivate async Task BuildStealthTargets()and await its callers.
🧹 Nitpick comments (4)
src/Producers/LdapProducer.cs (3)
73-80: Use culture-safe, allocation-light case-insensitive checks for DN filtering.
Avoid ToLower() and rely on OrdinalIgnoreCase; also anchor “user/machine” with a trailing comma to prevent false positives.- if (searchResult.TryGetDistinguishedName(out var distinguishedName)) { - var lower = distinguishedName.ToLower(); - if (lower.Contains("cn=domainupdates,cn=system")) - continue; - if (lower.Contains("cn=policies,cn=system") && (lower.StartsWith("cn=user") || lower.StartsWith("cn=machine"))) - continue; + if (searchResult.TryGetDistinguishedName(out var distinguishedName)) { + if (distinguishedName.IndexOf("CN=DomainUpdates,CN=System", StringComparison.OrdinalIgnoreCase) >= 0) + continue; + if (distinguishedName.IndexOf("CN=Policies,CN=System", StringComparison.OrdinalIgnoreCase) >= 0 && + (distinguishedName.StartsWith("CN=User,", StringComparison.OrdinalIgnoreCase) || + distinguishedName.StartsWith("CN=Machine,", StringComparison.OrdinalIgnoreCase))) + continue;
57-66: Guard against repeated partition errors to reduce log spam and wasted queries.
If one partition fails with a filter/parameter error, skip remaining partitions for that filter.- foreach (var partitionedFilter in GetPartitionedFilter(filter)) { - await foreach (var result in Context.LDAPUtils.PagedQuery(new LdapQueryParameters() { + var partitionFailed = false; + foreach (var partitionedFilter in GetPartitionedFilter(filter)) { + if (partitionFailed) break; + await foreach (var result in Context.LDAPUtils.PagedQuery(new LdapQueryParameters() { LDAPFilter = partitionedFilter, Attributes = ldapData.Attributes, DomainName = domain.Name, SearchBase = Context.SearchBase, IncludeSecurityDescriptor = Context.ResolvedCollectionMethods.HasFlag(CollectionMethod.ACL) }, cancellationToken)){ if (!result.IsSuccess) { Context.Logger.LogError("Error during main ldap query:{Message} ({Code})", result.Error, result.ErrorCode); - break; + partitionFailed = true; + break; }
90-91: Add a correctly-spelled alias property PartitionLdapQueries on Flags that forwards to the existing ParititonLdapQueries (keep the misspelled property for backward-compat).
- Action: add PartitionLdapQueries property to src/Client/Flags.cs so it gets/sets ParititonLdapQueries.
- Occurrences: src/Client/Flags.cs:30 (ParititonLdapQueries), src/Producers/LdapProducer.cs:90-91 (uses Context.Flags.ParititonLdapQueries), src/Options.cs:144 (defines PartitionLdapQueries), src/Sharphound.cs:88 (maps ParititonLdapQueries = options.PartitionLdapQueries).
- Optional: mark ParititonLdapQueries [Obsolete] to encourage migration to the correctly-spelled name.
src/Runtime/ObjectProcessors.cs (1)
154-161: Dictionary merge may throw on duplicate keys.
ContextUtils.Merge uses Concat().ToDictionary(), which throws on duplicates. Confirm no overlapping keys, or switch to a duplicate-tolerant merge.Possible safer Merge:
- return dict.Concat(delta).ToDictionary(kvp => kvp.Key, kvp => kvp.Value); + foreach (var kvp in delta) dict[kvp.Key] = kvp.Value; + return dict;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
Sharphound.csproj(1 hunks)src/BaseContext.cs(0 hunks)src/Client/Context.cs(0 hunks)src/Producers/LdapProducer.cs(3 hunks)src/Runtime/ObjectProcessors.cs(8 hunks)
💤 Files with no reviewable changes (2)
- src/BaseContext.cs
- src/Client/Context.cs
🧰 Additional context used
🧬 Code graph analysis (1)
src/Runtime/ObjectProcessors.cs (2)
src/Client/Context.cs (1)
ContextUtils(20-27)src/Client/Flags.cs (1)
Flags(5-31)
🔇 Additional comments (8)
Sharphound.csproj (1)
47-47: LGTM — no functional change.
Trailing newline only.src/Producers/LdapProducer.cs (3)
121-143: Config NC search path fallback looks good.
De-duping via configurationNCsCollected and SearchBase override are correct.
144-165: Explicit NamingContext fallback is solid.
Good resilience when Configuration NC DN cannot be resolved.
89-98: Validate objectGUID substring partitioning with your DCs.AD treats objectGUID as a binary (Octet String); substring filters may not be supported uniformly. Because this is gated by Context.Flags.ParititonLdapQueries, verify the partitioned filter returns results on your target domain controllers before enabling by default.
File: src/Producers/LdapProducer.cs (lines 89–98)
private IEnumerable<string> GetPartitionedFilter(string originalFilter) { if (Context.Flags.ParititonLdapQueries) { for (var i = 0; i < 256; i++) { yield return $"(&{originalFilter}(objectguid=\\{i.ToString("x2")}*))"; } } else { yield return originalFilter; } }The provided ripgrep run returned no matches in-repo; manual verification against your DCs (and a local repo search for other uses) is required.
src/Runtime/ObjectProcessors.cs (4)
125-129: LGTM — object initializer simplification.
Clear and idiomatic.
137-147: ACL enrichment looks good.
Owner-rights flags and isaclprotected property are consistent across types.
399-407: LGTM — ACL handling for groups mirrors other objects.
Consistent props and protection detection.
447-455: LGTM — domain inheritance hashes retained.
Good to keep for ACL diffing.
Reverts #165. This is being done because the feature has been delayed to a future release as it requires changes in Bloodhound as well.
Summary by CodeRabbit
Refactor
Style
Chores