Add CompStatus Events to CertAbuseProcessor BED-6967 #264
Conversation
WalkthroughRefactors registry access by removing static Helpers methods and introducing an injectable IRegistryAccessor/RegistryAccessor; updates SHRegistryKey/IRegistryKey surface; wires IRegistryAccessor into multiple processors; updates CertAbuseProcessor signatures and expands unit tests to mock and validate the new registry abstraction. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Processor as CertAbuseProcessor
participant Accessor as IRegistryAccessor / RegistryAccessor
participant Remote as RemoteMachine (Registry)
participant Logger as ILogger
Processor->>Accessor: GetRegistryKeyData(target, subkey, subvalue)
Note right of Accessor: opens connection (Connect) to hive asynchronously
Accessor->>Remote: Connect(hive, machineName)
Remote-->>Accessor: RegistryKey (or timeout/exception)
Accessor->>Accessor: Read value, handle IO/Security/Unauthorized
Accessor->>Logger: Debug or FailureReason when errors occur
Accessor-->>Processor: RegistryResult (Value, Collected, FailureReason)
Processor->>Logger: SendComputerStatus (success or failure) / continue processing
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
lrfalslev
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
src/CommonLib/Processors/ComputerSessionProcessor.cs (1)
27-27: Consider adding constructor injection for testability.The field type is correctly abstracted as
IRegistryAccessor, but the constructor always instantiates the concreteRegistryAccessor. This prevents mocking in unit tests. Consider adding an overload or optional parameter similar to other dependencies in this class.private readonly AdaptiveTimeout _readUserSessionsPriviledgedAdaptiveTimeout; -private readonly IRegistryAccessor _registryAccessor; +private readonly IRegistryAccessor _registryAccessor; public ComputerSessionProcessor(ILdapUtils utils, NativeMethods nativeMethods = null, ILogger log = null, string currentUserName = null, bool doLocalAdminSessionEnum = false, - string localAdminUsername = null, string localAdminPassword = null) { + string localAdminUsername = null, string localAdminPassword = null, + IRegistryAccessor registryAccessor = null) { _utils = utils; _nativeMethods = nativeMethods ?? new NativeMethods(); _currentUserName = currentUserName ?? WindowsIdentity.GetCurrent().Name.Split('\\')[1]; _log = log ?? Logging.LogProvider.CreateLogger("CompSessions"); _doLocalAdminSessionEnum = doLocalAdminSessionEnum; _localAdminUsername = localAdminUsername; _localAdminPassword = localAdminPassword; _readUserSessionsAdaptiveTimeout = new AdaptiveTimeout(maxTimeout: TimeSpan.FromMinutes(2), Logging.LogProvider.CreateLogger(nameof(ReadUserSessions))); _readUserSessionsPriviledgedAdaptiveTimeout = new AdaptiveTimeout(maxTimeout: TimeSpan.FromMinutes(2), Logging.LogProvider.CreateLogger(nameof(ReadUserSessionsPrivileged))); - _registryAccessor = new RegistryAccessor(); + _registryAccessor = registryAccessor ?? new RegistryAccessor();src/CommonLib/IRegistryKey.cs (1)
29-38: Remove commented-out code.Commented-out code should be deleted rather than left in the codebase. If
MockRegistryKeyis no longer needed (since tests now mockIRegistryAccessorinstead), remove it entirely.- // public class MockRegistryKey : IRegistryKey { - // public virtual object GetValue(string subkey, string name) { - // //Unimplemented - // return default; - // } - // - // public virtual string[] GetSubKeyNames() { - // throw new NotImplementedException(); - // } - // }src/CommonLib/IRegistryAccessor.cs (2)
53-55: Sync-over-async pattern may cause deadlocks.Using
.GetAwaiter().GetResult()on an async method can cause deadlocks in certain synchronization contexts. Consider makingGetRegistryKeyDataasync or providing a synchronousConnectoverload that doesn't use the async path.If the sync API is required for backward compatibility, consider extracting the core logic to avoid async wrapping:
public IRegistryKey OpenRemoteRegistry(string target) { - return Connect(RegistryHive.LocalMachine, target).GetAwaiter().GetResult(); + var remoteKey = _adaptiveTimeout.ExecuteWithTimeout((_) => RegistryKey.OpenRemoteBaseKey(RegistryHive.LocalMachine, target)).GetAwaiter().GetResult(); + if (remoteKey.IsSuccess) + return new SHRegistryKey(remoteKey.Value); + throw new TimeoutException($"Failed to connect to registry on {target}: {remoteKey.Error}"); }Alternatively, make
GetRegistryKeyDataasync to avoid the sync-over-async entirely.
17-18: Consider using a more descriptive logger name.The logger is created with
nameof(SHRegistryKey)but this is insideRegistryAccessor. Usingnameof(RegistryAccessor)would be more accurate.private static readonly AdaptiveTimeout _adaptiveTimeout = - new AdaptiveTimeout(maxTimeout: TimeSpan.FromSeconds(10), Logging.LogProvider.CreateLogger(nameof(SHRegistryKey))); + new AdaptiveTimeout(maxTimeout: TimeSpan.FromSeconds(10), Logging.LogProvider.CreateLogger(nameof(RegistryAccessor)));src/CommonLib/Processors/CertAbuseProcessor.cs (1)
211-216: Good null-safety guard for DiscretionaryAcl.This prevents a
NullReferenceExceptionwhen iterating the DACL. However, consider whether this should be logged or reported as a potential issue rather than silently returning an empty result.if (descriptor.DiscretionaryAcl is null) { + _log.LogDebug("DiscretionaryAcl is null for CA {CAName} on {ComputerName}", caName, computerName); return ret; }test/unit/CertAbuseProcessorTest.cs (2)
90-95: Inconsistentnameofusage pattern.Success tests use
nameof(CertAbuseProcessor.IsUserSpecifiesSanEnabled)(line 66) while failure tests usenameof(_certAbuseProcessor.IsUserSpecifiesSanEnabled)(line 92). Both work, but prefer consistent usage of the class name version for clarity.- Assert.Equal(nameof(_certAbuseProcessor.IsUserSpecifiesSanEnabled), _receivedCompStatus.Task); + Assert.Equal(nameof(CertAbuseProcessor.IsUserSpecifiesSanEnabled), _receivedCompStatus.Task);Same applies to lines 147, 247, and 318.
388-426: Consider removing or implementing commented-out tests.These commented tests appear to be legacy code. Either update them to work with the new constructor or remove them to avoid dead code accumulation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/CommonLib/Helpers.cs(1 hunks)src/CommonLib/IRegistryAccessor.cs(1 hunks)src/CommonLib/IRegistryKey.cs(2 hunks)src/CommonLib/Processors/CertAbuseProcessor.cs(10 hunks)src/CommonLib/Processors/ComputerSessionProcessor.cs(3 hunks)src/CommonLib/Processors/DCRegistryProcessor.cs(4 hunks)test/unit/CertAbuseProcessorTest.cs(2 hunks)test/unit/Facades/MockLdapUtils.cs(1 hunks)test/unit/Utils.cs(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-10-17T13:43:46.833Z
Learnt from: MikeX777
Repo: SpecterOps/SharpHoundCommon PR: 241
File: src/CommonLib/Processors/LdapPropertyProcessor.cs:168-169
Timestamp: 2025-10-17T13:43:46.833Z
Learning: Properties added to dictionaries returned by methods in SharpHoundCommon (such as those in LdapPropertyProcessor) may be consumed by dependent projects like SharpHound (SH) and SharpHoundEnterprise (SHE), even if they are not used within the SharpHoundCommon repository itself.
Applied to files:
src/CommonLib/Processors/CertAbuseProcessor.cs
📚 Learning: 2025-07-15T17:45:25.688Z
Learnt from: definitelynotagoblin
Repo: SpecterOps/SharpHoundCommon PR: 222
File: src/CommonLib/Processors/LocalGroupProcessor.cs:19-27
Timestamp: 2025-07-15T17:45:25.688Z
Learning: In SharpHoundCommon, the team prefers to keep code simple rather than implement perfect resource management when the resources being managed are non-critical. Specifically, they accept not implementing IDisposable for AdaptiveTimeout instances when the Dispose method is primarily for flushing analytics logs from ExecutionTimeSampler, viewing it as a courtesy rather than a safety requirement.
Applied to files:
src/CommonLib/IRegistryKey.cs
🧬 Code graph analysis (5)
src/CommonLib/IRegistryAccessor.cs (3)
src/CommonLib/Helpers.cs (1)
Task(285-300)src/CommonLib/IRegistryKey.cs (4)
SHRegistryKey(10-27)SHRegistryKey(13-15)GetValue(6-6)GetValue(17-20)src/SharpHoundRPC/Registry/RemoteRegistryStrategy.cs (1)
RegistryKey(61-72)
test/unit/Utils.cs (1)
test/unit/CertAbuseProcessorTest.cs (2)
WindowsOnlyTheory(187-224)WindowsOnlyTheory(344-363)
src/CommonLib/Processors/DCRegistryProcessor.cs (2)
src/CommonLib/Processors/GPOLocalGroupProcessor.cs (2)
Task(52-58)Task(60-249)src/CommonLib/IRegistryAccessor.cs (1)
RegistryAccessor(16-75)
src/CommonLib/Processors/ComputerSessionProcessor.cs (1)
src/CommonLib/IRegistryAccessor.cs (1)
RegistryAccessor(16-75)
test/unit/CertAbuseProcessorTest.cs (5)
test/unit/Utils.cs (4)
WindowsOnlyTheory(94-100)WindowsOnlyTheory(96-99)WindowsOnlyFact(86-92)WindowsOnlyFact(88-91)src/CommonLib/IRegistryAccessor.cs (2)
RegistryResult(11-11)RegistryResult(20-51)src/CommonLib/Processors/RegistryResult.cs (1)
RegistryResult(4-6)src/CommonLib/SecurityDescriptor.cs (3)
List(83-92)ActiveDirectorySecurityDescriptor(69-114)ActiveDirectorySecurityDescriptor(73-76)src/CommonLib/ILdapUtils.cs (1)
ActiveDirectorySecurityDescriptor(126-126)
🔇 Additional comments (17)
test/unit/Facades/MockLdapUtils.cs (1)
747-747: LGTM!Minor whitespace formatting change. No functional impact.
src/CommonLib/Processors/ComputerSessionProcessor.cs (1)
298-298: LGTM!The
usingstatement properly disposes theIRegistryKeyafter use, andRegistryHive.Usersis correct for enumerating user sessions from the registry.src/CommonLib/Processors/DCRegistryProcessor.cs (2)
20-22: LGTM with noted TODO.The TODO acknowledges the need for dependency injection to enable unit testing. The implementation is correct and consistent with the pattern used in other processors.
37-37: LGTM!Clean transition from static helper to injected dependency.
test/unit/Utils.cs (1)
94-100: LGTM!Clean implementation that mirrors the existing
WindowsOnlyFactpattern, providing aTheoryAttributevariant for parameterized tests that should only run on Windows.src/CommonLib/Helpers.cs (1)
149-156: LGTM!Minor documentation formatting change. The removal of registry access methods from this file aligns with the refactoring to use the new
IRegistryAccessorabstraction.src/CommonLib/IRegistryKey.cs (1)
5-5: LGTM!Adding
IDisposableto the interface is correct sinceIRegistryKeywraps aRegistryKeywhich is a disposable resource.src/CommonLib/IRegistryAccessor.cs (1)
10-14: LGTM!Interface design is clean and provides the necessary abstraction for registry access operations.
src/CommonLib/Processors/CertAbuseProcessor.cs (5)
21-32: Good refactoring to dependency injection for registry access.The constructor now accepts
IRegistryAccessorwhich improves testability and follows dependency inversion principle. This is a clean approach for abstracting remote registry access.
51-67: Status reporting adds good observability.The pattern of reporting both failure and success statuses is consistent throughout the file. This will help with debugging remote registry access issues in production.
469-471: Good null check for opaque data.Prevents
ArgumentNullExceptionfromBitConverter.ToUInt32whenGetOpaque()returns null for non-callback ACEs.
287-325: Method correctly made async for status reporting.The async signature is appropriate due to
SendComputerStatusawaits. Note that_registryAccessor.GetRegistryKeyDatais still synchronous blocking I/O, but this is consistent with the pattern used elsewhere in this file and may be acceptable given the nature of remote registry access.
256-276: Clean abstraction for registry access.Both methods now consistently use
_registryAccessor.GetRegistryKeyData, enabling proper mocking in tests.test/unit/CertAbuseProcessorTest.cs (4)
30-40: Good test setup with event capture pattern.The event handler approach for capturing
ComputerStatusEventis clean and enables verification of status reporting behavior.
152-184: Good test data coverage for DACL edge cases.The test data provider covers important scenarios: null DACL, empty DACL, null opaque, and empty opaque. This aligns well with the null-safety guards added in the processor.
186-187: TODO comment tracks SAM server mocking need.The TODO appropriately documents the limitation of needing to mock the SAM server for SID lookups. This is acceptable for now.
344-362: Good coverage for filtered SID patterns.Tests verify that well-known filtered SID prefixes (NT SERVICE, IIS APPPOOL, etc.) are correctly rejected by
GetRegistryPrincipal.
|
Add the BED ticket (either link or "BED-XXXX") to the PR's motivation and context - it makes it easier to track for release notes and history and such. |
definitelynotagoblin
left a comment
definitelynotagoblin
left a comment
There was a problem hiding this comment.
Check the coderabbit comment, I think it's on to something.
|
Excellent work! very minor nits, but I think this is very much in a good direction. |
lrfalslev
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/CommonLib/IRegistryAccessor.cs (1)
59-61: Blocking call on async method.
GetAwaiter().GetResult()performs a blocking wait on an async operation. While this is generally acceptable in CLI contexts like SharpHound, be aware this would be problematic on synchronization contexts with thread affinity (e.g., UI threads, ASP.NET Core request threads without explicit ConfigureAwait).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/CommonLib/IRegistryAccessor.cs(1 hunks)src/CommonLib/IRegistryKey.cs(1 hunks)src/CommonLib/Processors/CertAbuseProcessor.cs(10 hunks)src/CommonLib/Processors/DCRegistryProcessor.cs(4 hunks)test/unit/CertAbuseProcessorTest.cs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/CommonLib/Processors/DCRegistryProcessor.cs
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-07-15T17:45:25.688Z
Learnt from: definitelynotagoblin
Repo: SpecterOps/SharpHoundCommon PR: 222
File: src/CommonLib/Processors/LocalGroupProcessor.cs:19-27
Timestamp: 2025-07-15T17:45:25.688Z
Learning: In SharpHoundCommon, the team prefers to keep code simple rather than implement perfect resource management when the resources being managed are non-critical. Specifically, they accept not implementing IDisposable for AdaptiveTimeout instances when the Dispose method is primarily for flushing analytics logs from ExecutionTimeSampler, viewing it as a courtesy rather than a safety requirement.
Applied to files:
src/CommonLib/IRegistryKey.cs
📚 Learning: 2025-10-17T13:43:46.833Z
Learnt from: MikeX777
Repo: SpecterOps/SharpHoundCommon PR: 241
File: src/CommonLib/Processors/LdapPropertyProcessor.cs:168-169
Timestamp: 2025-10-17T13:43:46.833Z
Learning: Properties added to dictionaries returned by methods in SharpHoundCommon (such as those in LdapPropertyProcessor) may be consumed by dependent projects like SharpHound (SH) and SharpHoundEnterprise (SHE), even if they are not used within the SharpHoundCommon repository itself.
Applied to files:
src/CommonLib/Processors/CertAbuseProcessor.cs
🧬 Code graph analysis (4)
src/CommonLib/IRegistryAccessor.cs (2)
src/CommonLib/IRegistryKey.cs (4)
GetValue(6-6)GetValue(17-20)SHRegistryKey(10-27)SHRegistryKey(13-15)src/SharpHoundRPC/Registry/RemoteRegistryStrategy.cs (1)
RegistryKey(61-72)
src/CommonLib/IRegistryKey.cs (2)
src/CommonLib/IRegistryAccessor.cs (2)
IRegistryKey(12-12)IRegistryKey(59-61)src/SharpHoundRPC/Registry/RemoteRegistryStrategy.cs (1)
RegistryKey(61-72)
src/CommonLib/Processors/CertAbuseProcessor.cs (2)
src/CommonLib/CSVComputerStatus.cs (1)
CSVComputerStatus(6-47)src/CommonLib/OutputTypes/APIResults/BoolRegistryAPIResult.cs (1)
BoolRegistryAPIResult(3-6)
test/unit/CertAbuseProcessorTest.cs (4)
src/CommonLib/IRegistryAccessor.cs (2)
RegistryResult(11-11)RegistryResult(25-57)src/CommonLib/Processors/RegistryResult.cs (1)
RegistryResult(4-6)src/CommonLib/SecurityDescriptor.cs (3)
List(83-92)ActiveDirectorySecurityDescriptor(69-114)ActiveDirectorySecurityDescriptor(73-76)src/CommonLib/ILdapUtils.cs (1)
ActiveDirectorySecurityDescriptor(126-126)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (20)
src/CommonLib/IRegistryKey.cs (2)
5-5: LGTM! Good addition of IDisposable.Extending
IRegistryKeywithIDisposableensures proper cleanup of registry handles and aligns with the resource management pattern used throughout the codebase.
13-15: LGTM! Constructor visibility change supports the new pattern.Making the constructor public enables instantiation from the
RegistryAccessorimplementation, which is necessary for the new centralized registry access pattern introduced in this PR.src/CommonLib/IRegistryAccessor.cs (3)
20-23: LGTM! Good use of optional logger parameter with sensible default.The constructor properly initializes both the logger and adaptive timeout, with a fallback logger if none is provided.
28-34: Excellent fix! Resource leak addressed.The
usingstatement properly disposes thebaseKeyafter use, addressing the resource leak concern raised in previous review comments.Based on learnings, the team values fixing resource management issues during refactoring.
75-80: LGTM! Clear timeout handling with informative exception.The method properly handles timeout scenarios and provides a descriptive error message including both the target machine and the underlying error.
src/CommonLib/Processors/CertAbuseProcessor.cs (8)
21-21: LGTM! Clean dependency injection of registry accessor.The
IRegistryAccessoris properly injected via constructor, enabling centralized registry access and making the class more testable.Also applies to: 26-28
43-67: LGTM! Comprehensive status reporting for both success and failure paths.The addition of
computerObjectIdparameter andSendComputerStatuscalls on both failure (lines 51-56) and success (lines 62-67) provides excellent observability for registry enrollment permission collection.
177-201: LGTM! Consistent status reporting pattern.The status reporting follows the same pattern as
ProcessRegistryEnrollmentPermissions, providing consistent observability across the processor.
212-215: LGTM! Good defensive null check.Checking
descriptor.DiscretionaryAclfor null preventsNullReferenceExceptionwhen the registry data is incomplete or malformed. This guard is appropriate given the external data source.
287-314: LGTM! Method signature update with proper async pattern and status reporting.The method is now properly async and includes
computerObjectIdfor status reporting. The status events are emitted for both failure (lines 298-303) and success (lines 309-314) paths.
337-363: LGTM! Consistent async pattern with status reporting.Similar to
IsUserSpecifiesSanEnabled, this method now properly reports status for both success and failure paths.
469-471: LGTM! Essential null check for opaque data.Checking
opaquefor null before attempting to read from it prevents crashes when ACE data is malformed. This defensive check is appropriate for data coming from external registry sources.
256-262: LGTM! Refactored to use centralized registry accessor.Both
GetCASecurityandGetEnrollmentAgentRightsnow delegate to_registryAccessor.GetRegistryKeyDatainstead of calling static helpers, which improves testability and consolidates registry access logic.Also applies to: 270-276
test/unit/CertAbuseProcessorTest.cs (7)
18-40: LGTM! Well-structured test setup.The test class properly initializes mocks for both
ILdapUtilsandIRegistryAccessor, and captures theComputerStatusEventfor verification. This setup supports thorough testing of the new status reporting functionality.
42-68: LGTM! Comprehensive test with dual verification.The test validates both the functional result (lines 59-61) and the status event (lines 64-67), ensuring that the
IsUserSpecifiesSanEnabledmethod behaves correctly end-to-end.
148-180: LGTM! Excellent parameterized test data covering edge cases.The test data covers important edge cases:
- Null DACL
- Empty DACL
- DACL with null opaque data
- DACL with empty opaque data
This ensures robust handling of malformed or incomplete registry data.
182-218: LGTM! Effective use of parameterized testing.Using
WindowsOnlyTheorywithMemberDataallows testing multiple edge cases with a single test method, improving maintainability while ensuring comprehensive coverage of theProcessEAPermissionsmethod.
245-286: LGTM! Good test of empty data scenario.The test validates that when the registry contains a valid but empty security descriptor (no owner, no rules), the method returns successfully with empty data rather than failing. This is correct behavior.
313-332: LGTM! Test validates template resolution logic.The test properly verifies that
ProcessCertTemplatescorrectly partitions templates into resolved and unresolved collections based on the LDAP lookup results.
334-352: LGTM! Test validates SID filtering.The parameterized test ensures that filtered SIDs (service SIDs) are correctly rejected by
GetRegistryPrincipal, which is important for avoiding unnecessary processing of non-relevant principals.
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
Checklist:
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.