Add CompStatus Events to CertAbuseProcessor BED-6967

[lrfalslev]

Description

Update CertAbuseProcessor to write contacted machines to the compstatus log.

Motivation and Context

BED-6967

How Has This Been Tested?

Added new unit tests and manually tested SHCE and SHE in a lab.

Screenshots (if appropriate):

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

• Refactor

  • Centralized status reporting into an event-driven flow to simplify internal signaling and reduce parameter chaining.
  • Improved event handler lifecycle with explicit cleanup after processing to prevent leaks and duplicate notifications.

• Bug Fixes

  • Fixed stray/formatting noise and ensured consistent status emission paths for more reliable progress reporting.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

Walkthrough

The change centralizes CSVComputerStatus reporting inside ObjectProcessors by adding a Channel dependency to its constructor, removing the per-call channel parameter from processing methods, and adding ClearEventHandlers usage at caller sites (CollectionTask, LDAPConsumer).

Changes

Cohort / File(s)	Summary
ObjectProcessors refactor src/Runtime/ObjectProcessors.cs	Constructor signature changed to accept Channel<CSVComputerStatus> compStatusChannel; stores channel and wires internal event handlers that forward status via HandleCompStatusEvent. Removed channel parameter from ProcessObject and related methods. Replaced direct channel writes with event-based emissions. Added ClearEventHandlers() to detach handlers.
Call-site updates src/Runtime/CollectionTask.cs, src/Runtime/LDAPConsumer.cs	Updated ObjectProcessors instantiation to pass _compStatusChannel. Updated processor.ProcessObject(...) calls to the new signature (channel argument removed). Added processor.ClearEventHandlers() after processing/batch completion. Minor whitespace fix in StartCollection.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

• Review event wiring and removal in ObjectProcessors.cs (handler attachment/detachment).
• Validate HandleCompStatusEvent async error handling and channel write concurrency/thread-safety.
• Confirm all prior direct channel writes were correctly replaced and no status paths were missed.
• Inspect call sites (CollectionTask, LDAPConsumer) for proper lifecycle handling (ClearEventHandlers placement).

Possibly related PRs

• Add CompStatus Writing Locations #161 — similar changes to CSVComputerStatus propagation and ObjectProcessors constructor/signature updates.

Suggested reviewers

• ktstrader
• definitelynotagoblin

Poem

🐰 I hopped through code with tiny paws,

Channels tucked inside one cause,
Handlers bound, then cleared with care,
Status hops through events in the air,
A rabbit's cheer for tidy software! ✨

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 clearly refers to the main change: adding CompStatus events to CertAbuseProcessor, which aligns with the core refactoring of status reporting from direct channel writes to event-driven mechanisms.
Description check	✅ Passed	The PR description includes all critical sections: clear description of changes, motivation with issue link (BED-6967), testing details (new tests added, manual lab testing), change type selected, and key checklist items marked. Description is complete and follows the template structure.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch lfalslev/bed-6967

---

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e6da9e7 and 6eb6525.

📒 Files selected for processing (1)

• src/Runtime/ObjectProcessors.cs (10 hunks)

🧰 Additional context used

🧬 Code graph analysis (1)

src/Runtime/ObjectProcessors.cs (2)

src/Producers/ComputerFileProducer.cs (2)

• Task (32-119)
• Task (122-126)

src/Runtime/CollectionTask.cs (2)

• Task (64-109)
• Task (111-153)

🔇 Additional comments (8)

src/Runtime/ObjectProcessors.cs (8)

43-45: LGTM! Clean dependency injection.

The addition of _compStatusChannel field and constructor parameter follows standard dependency injection patterns and enables centralized computer status reporting.

---

79-87: LGTM! Proper event handler cleanup.

The ClearEventHandlers method correctly mirrors the subscriptions in the constructor and is called at appropriate cleanup points (verified in related files). This prevents memory leaks from lingering event subscriptions.

---

89-95: LGTM! Robust event handler implementation.

The error handling appropriately prevents exceptions in the event handler from propagating to callers, and the cancellation token usage allows for graceful shutdown. The Channel writer is thread-safe, so concurrent calls from multiple processors are handled correctly.

---

97-97: LGTM! Consistent signature simplification.

Removing the compStatusChannel parameter from ProcessObject is consistent with the centralized event-driven approach where the channel is now a constructor dependency rather than a per-call parameter.

---

310-310: LGTM! Consistent migration to event-driven status reporting.

The replacement of direct channel writes with HandleCompStatusEvent calls is consistent throughout the computer processing logic. The conditional status reporting (guarded by DumpComputerStatus flag) is preserved correctly.

Also applies to: 326-331, 342-347, 355-360

---

776-782: LGTM! Fulfills PR objective to add CompStatus events.

The addition of computer status events when resolving EnterpriseCA hosting computers aligns with the PR objective. Both collection paths (CertServices and CARegistry) consistently report success status, enabling proper tracking of contacted machines.

Also applies to: 805-811

---

818-821: Verify that IsUserSpecifiesSanEnabled, ProcessEAPermissions, IsRoleSeparationEnabled, and ProcessRegistryEnrollmentPermissions handle null hostingComputer parameter.

The ret.HostingComputer parameter passed to these methods at lines 818, 820, 821, and 826 can be null. It is only assigned when ResolveHostToSid succeeds and returns a valid SID starting with "S-1-" (lines 803-815). If host resolution fails, ret.HostingComputer remains at its default value (null for reference types) and is passed to external methods. These methods are from the SharpHoundCommon NuGet package and should handle null values gracefully to avoid null reference exceptions.

---

70-76: Verify that all processors emitting ComputerStatusEvent are subscribed.

The processors are from the external SharpHoundCommon NuGet package (version 4.5.1). Currently subscribed to ComputerStatusEvent: _localGroupProcessor, _computerSessionProcessor, _userRightsAssignmentProcessor, _computerAvailability, _spnProcessor, _ldapPropertyProcessor, and _certAbuseProcessor.

The following processors are initialized but not subscribed: _aclProcessor, _containerProcessor, _dcRegistryProcessor, _domainTrustProcessor, _groupProcessor, _gpoLocalGroupProcessor, _webClientProcessor, and _smbProcessor. Confirm whether any of these should emit ComputerStatusEvent and be added to the subscription list.

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[coderabbitai]

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)

764-799: Inconsistent ObjectId in status events.

There's an inconsistency between the two status events in ProcessEnterpriseCA:

• Line 769: Uses resolvedSearchResult.ObjectId (the CA's object ID)
• Line 798: Uses sid (the hosting computer's SID)

Both events report the same task (ProcessEnterpriseCA) but with different ObjectId values. If both CertServices and CARegistry collection methods are enabled, this could cause confusion in status output.

Consider aligning the ObjectId values:

                        await HandleCompStatusEvent(new CSVComputerStatus
                        {
                            Status = ComputerStatus.Success,
                            ComputerName = resolvedSearchResult.DisplayName,
                            Task = nameof(ProcessEnterpriseCA),
-                            ObjectId = sid,
+                            ObjectId = resolvedSearchResult.ObjectId,
                        });

🧹 Nitpick comments (1)

src/Runtime/ObjectProcessors.cs (1)

756-830: Consider deduplicating host resolution logic.

If both CertServices and CARegistry collection methods are enabled, the same dnsHostName is resolved twice (lines 761-763 and 791-792), and ret.HostingComputer is set twice. This could be refactored to resolve once at the start.

One approach would be to hoist the host resolution outside of the flag checks:

        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;
                    // ... status event and enrollment processing
-               }
            }
        }

-       if (_methods.HasFlag(CollectionMethod.CARegistry)) {
+       if (_methods.HasFlag(CollectionMethod.CARegistry) && ret.HostingComputer == null) {
            // Only resolve if not already done above

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ed84a79 and e6da9e7.

📒 Files selected for processing (3)

• src/Runtime/CollectionTask.cs (3 hunks)
• src/Runtime/LDAPConsumer.cs (3 hunks)
• src/Runtime/ObjectProcessors.cs (10 hunks)

🧰 Additional context used

🧬 Code graph analysis (3)

src/Runtime/LDAPConsumer.cs (1)

src/Runtime/ObjectProcessors.cs (3)

• ObjectProcessors (21-939)
• ObjectProcessors (45-71)
• ClearEventHandlers (73-75)

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)

src/Runtime/CollectionTask.cs (1)

src/Runtime/ObjectProcessors.cs (3)

• ObjectProcessors (21-939)
• ObjectProcessors (45-71)
• ClearEventHandlers (73-75)

🔇 Additional comments (5)

src/Runtime/CollectionTask.cs (1)

114-114: LGTM - Event-driven refactor correctly applied.

The changes properly integrate with the new status reporting architecture:

• Channel passed to constructor for internal event handling
• ProcessObject signature updated (channel no longer passed per-call)
• Event handlers cleared after processing completes to prevent leaks

Also applies to: 132-132, 149-150

src/Runtime/LDAPConsumer.cs (1)

21-21: LGTM - Consistent with CollectionTask.cs refactor.

The event-driven status reporting pattern is correctly applied here, matching the implementation in CollectionTask.cs.

Also applies to: 37-37, 54-55

src/Runtime/ObjectProcessors.cs (3)

43-75: LGTM - Clean event subscription pattern.

The constructor properly stores the channel and wires up the event handler. ClearEventHandlers() correctly unsubscribes to prevent memory leaks.

---

297-349: LGTM - Consistent status event reporting in computer processing.

All status events in ProcessComputerObject properly use HandleCompStatusEvent and maintain consistent structure with resolvedSearchResult.ObjectId.

---

77-83: Verify the event delegate type in SharpHoundCommonLib.Processors.CertAbuseProcessor.

The implementation correctly handles exceptions to prevent status reporting failures from crashing the processor. However, ensure that CertAbuseProcessor.ComputerStatusEvent supports async handlers (e.g., event EventHandler<CSVComputerStatus> where EventHandler is async-compatible), otherwise the async method may fire-and-forget. Since this type is from the external SharpHoundCommonLib package, check the library documentation or source to confirm the delegate signature.

[mykeelium]

great work, I think this can be brought in. Could you also add the other processors delegate subscriptions in ObjectProcessors.cs?

[mykeelium]

Fantastic!