Collect EnrollmentAgent Restrictions For AllTemplates, add CertAbuseProcessor Tests BED-7044 #192
Conversation
WalkthroughThe PR refactors CSVComputerStatus channel propagation across the codebase. Instead of passing the compStatus channel as a method parameter to ProcessObject, the channel is now injected into ObjectProcessors via constructor dependency injection. Event subscriptions handle status updates, and a new ClearEventHandlers method manages lifecycle cleanup. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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
🧹 Nitpick comments (1)
src/Runtime/ObjectProcessors.cs (1)
769-843: RedundantHostingComputerresolution when bothCertServicesandCARegistryflags are set.The
HostingComputerresolution and assignment occurs in both theCertServicesblock (lines 774-786) and theCARegistryblock (lines 804-816). When both flags are enabled, this results in:
- Duplicate DNS-to-SID resolution calls
- Two status events emitted for the same CA
Consider extracting the host resolution to run once before these blocks and reusing the result.
♻️ Suggested refactor
+ // Resolve hosting computer once for both CertServices and CARegistry + string hostingSid = null; + var caName = entry.GetProperty(LDAPProperties.Name); + var dnsHostName = entry.GetProperty(LDAPProperties.DNSHostName); + if (caName != null && dnsHostName != null) { + if (await _context.LDAPUtils.ResolveHostToSid(dnsHostName, resolvedSearchResult.DomainSid) is + (true, var sid) && sid.StartsWith("S-1-")) { + hostingSid = sid; + ret.HostingComputer = hostingSid; + await HandleCompStatusEvent(new CSVComputerStatus { + Status = ComputerStatus.Success, + ComputerName = resolvedSearchResult.DisplayName, + Task = nameof(ProcessEnterpriseCA), + ObjectId = resolvedSearchResult.ObjectId, + }); + } else { + _log.LogWarning("CA {Name} host ({Dns}) could not be resolved to a SID.", caName, dnsHostName); + } + } + if (_methods.HasFlag(CollectionMethod.CertServices)) { - var caName = entry.GetProperty(LDAPProperties.Name); - var dnsHostName = entry.GetProperty(LDAPProperties.DNSHostName); - if (caName != null && dnsHostName != null) { - if (await _context.LDAPUtils.ResolveHostToSid(dnsHostName, resolvedSearchResult.DomainSid) is - (true, var sid) && sid.StartsWith("S-1-")) { - ret.HostingComputer = sid; - await HandleCompStatusEvent(new CSVComputerStatus - { - Status = ComputerStatus.Success, - ... - }); - } else { - _log.LogWarning("CA {Name} host ({Dns}) could not be resolved to a SID.", caName, dnsHostName); - } var caEnrollmentProcessor = new CAEnrollmentProcessor(dnsHostName, caName, _log); ... } } if (_methods.HasFlag(CollectionMethod.CARegistry)) { - var caName = entry.GetProperty(LDAPProperties.Name); - var dnsHostName = entry.GetProperty(LDAPProperties.DNSHostName); if (caName != null && dnsHostName != null) { - if (await _context.LDAPUtils.ResolveHostToSid(...) { ... } CARegistryData cARegistryData = new() { - IsUserSpecifiesSanEnabled = await _certAbuseProcessor.IsUserSpecifiesSanEnabled(dnsHostName, caName, ret.HostingComputer), + IsUserSpecifiesSanEnabled = await _certAbuseProcessor.IsUserSpecifiesSanEnabled(dnsHostName, caName, hostingSid), ... }; } }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/Runtime/CollectionTask.cssrc/Runtime/LDAPConsumer.cssrc/Runtime/ObjectProcessors.cs
🧰 Additional context used
🧬 Code graph analysis (2)
src/Runtime/CollectionTask.cs (1)
src/Runtime/ObjectProcessors.cs (3)
ObjectProcessors(22-952)ObjectProcessors(46-78)ClearEventHandlers(80-88)
src/Runtime/ObjectProcessors.cs (2)
src/Runtime/CollectionTask.cs (2)
Task(64-109)Task(111-153)src/Producers/ComputerFileProducer.cs (2)
Task(32-119)Task(122-126)
🔇 Additional comments (14)
src/Runtime/LDAPConsumer.cs (3)
21-21: LGTM!The constructor now correctly passes the
computerStatusChanneltoObjectProcessors, aligning with the new dependency injection pattern for centralized status reporting.
37-37: LGTM!The
ProcessObjectcall correctly omits thecompStatusChannelparameter, consistent with the refactored method signature that now uses event-driven status reporting internally.
54-55: LGTM!
ClearEventHandlers()is appropriately called after theforeachloop completes to detach event subscriptions and prevent memory leaks. The placement outside the loop ensures cleanup occurs once per consumer lifecycle.src/Runtime/ObjectProcessors.cs (8)
44-77: LGTM - Clean event-driven status reporting setup.The constructor properly:
- Stores the channel reference for centralized status writing
- Subscribes to
ComputerStatusEventon all relevant processorsThe event subscriptions are symmetric with
ClearEventHandlers(), ensuring proper lifecycle management.
80-88: LGTM!The
ClearEventHandlers()method correctly unsubscribes from all events that were subscribed in the constructor, preventing memory leaks and ensuring clean lifecycle management.
98-132: LGTM!The
ProcessObjectmethod signature correctly removes thecompStatusChannelparameter, and the switch statement properly delegates to the refactored processing methods.
238-241: LGTM!
ProcessComputerObjectsignature correctly updated to remove thecompStatusChannelparameter.
311-332: LGTM!Status reporting for computer availability and session enumeration correctly uses
HandleCompStatusEventinstead of direct channel writes.
343-361: LGTM!Privileged sessions and registry sessions status reporting correctly migrated to the event-driven pattern via
HandleCompStatusEvent.
734-734: LGTM!
ProcessEnterpriseCAsignature correctly updated to remove thecompStatusChannelparameter.
90-96: The event handler signature is correct. TheHandleCompStatusEventmethod'sasync Taskreturn type is properly designed for event handlers that need asynchronous behavior. Since this is production code (version 2.8.1) that compiles successfully, theComputerStatusEventdelegate signature fromSharpHoundCommonLib.Processorsmust be compatible with aFunc<CSVComputerStatus, Task>or similar async-aware delegate. C# enforces delegate/handler signature compatibility at compile-time, so a mismatch would have prevented compilation. The implementation correctly awaits the asynchronous operation and handles exceptions appropriately.Likely an incorrect or invalid review comment.
src/Runtime/CollectionTask.cs (3)
114-114: LGTM!The
ObjectProcessorsinstantiation correctly passes_compStatusChannelto the constructor, consistent with the new dependency injection pattern.
132-132: LGTM!The
ProcessObjectcall correctly uses the updated signature without thecompStatusChannelparameter.
149-150: LGTM!
ClearEventHandlers()is appropriately called after the consumer loop completes, ensuring event subscriptions are cleaned up and preventing memory leaks.
Description
Collect EnrollmentAgent Restrictions For AllTemplates, add CertAbuseProcessor Tests
Motivation and Context
BED-7044
How Has This Been Tested?
See: Common PR
Screenshots (if appropriate):
Types of changes
Checklist:
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.