Add CompStatus Events to CertAbuseProcessor BED-6967 #264
Conversation
WalkthroughRefactors remote registry and SAM access into injectable accessors (IRegistryAccessor, ISAMServerAccessor), removes static Helpers/SAMServer.OpenServer helpers and SHRegistryKey.Connect, updates processors to use accessors, and expands CertAbuseProcessor tests and status-reporting signatures. 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: async Connect then read value
Accessor->>Remote: Connect(hive, machineName)
Remote-->>Accessor: RegistryKey or error/timeout
Accessor->>Accessor: Read value, handle IO/Security/Unauthorized
Accessor->>Logger: Log debug and set FailureReason
Accessor-->>Processor: RegistryResult(Value, Collected, FailureReason)
Processor->>Logger: SendComputerStatus(success/failure, computerObjectId)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ 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 |
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.
…rocessor Tests BED-7044 (#268) * added tests for helpers and init common lib (#265) * added tests for helpers and init common lib * added a comment * coderabbit suggestion * removed comment * made some test the were using cache run in sequence so they dont step on eachother * made some test the were using cache run in sequence so they dont step on eachother * cleanup the UT a bit * Include Membership property set in AddSelf and AddMember edges (#266) * feat: add membership property set BED-7069 * Test GetRegistryPrincipal * test OpenSAMServer * test GetMachineSid * return EAPermission restriction when AllCertificates is true * finish CreateEnrollmentAgentRestriction tests * fix tests --------- Co-authored-by: Emmanuel Robles <[email protected]> Co-authored-by: Jim Sykora <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@test/unit/CertAbuseProcessorTest.cs`:
- Around line 625-629: The test asserts use ComputerStatus.Success
inconsistently; change the assertion in CertAbuseProcessorTest that checks
_receivedCompStatus.Status (which currently expects ComputerStatus.Success) to
use CSVComputerStatus.StatusSuccess instead so it matches other tests; update
the assertion that references ComputerStatus.Success to expect
CSVComputerStatus.StatusSuccess while keeping the other assertions for
_receivedCompStatus.ComputerName, Task
(nameof(_certAbuseProcessor.GetMachineSid)), and ObjectId unchanged.
🧹 Nitpick comments (1)
src/CommonLib/Processors/LocalGroupProcessor.cs (1)
29-31: Consider acceptingISAMServerAccessorvia constructor for testability.
CertAbuseProcessoracceptsISAMServerAccessorvia constructor injection, enabling mocking in tests. Here,SAMServerAccessoris instantiated directly, limiting the ability to unit testLocalGroupProcessorin isolation.♻️ Suggested refactor
-public LocalGroupProcessor(ILdapUtils utils, ILogger log = null) { +public LocalGroupProcessor(ILdapUtils utils, ISAMServerAccessor samServerAccessor = null, ILogger log = null) { _utils = utils; - _samServerAccessor = new SAMServerAccessor(); + _samServerAccessor = samServerAccessor ?? new SAMServerAccessor(); _log = log ?? Logging.LogProvider.CreateLogger("LocalGroupProcessor");
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/CommonLib/Processors/CertAbuseProcessor.cssrc/CommonLib/Processors/LocalGroupProcessor.cssrc/SharpHoundRPC/SAMRPCNative/SAMMethods.cssrc/SharpHoundRPC/Wrappers/ISAMServerAccessor.cssrc/SharpHoundRPC/Wrappers/SAMServer.cstest/unit/CertAbuseProcessorTest.cs
💤 Files with no reviewable changes (2)
- src/SharpHoundRPC/Wrappers/SAMServer.cs
- src/SharpHoundRPC/SAMRPCNative/SAMMethods.cs
🧰 Additional context used
🧠 Learnings (1)
📚 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 (1)
src/CommonLib/Processors/CertAbuseProcessor.cs (2)
src/CommonLib/Processors/GPOLocalGroupProcessor.cs (2)
TypedPrincipal(514-519)ToString(543-546)src/CommonLib/Processors/LocalGroupProcessor.cs (1)
SharpHoundRPC(45-54)
🔇 Additional comments (13)
src/SharpHoundRPC/Wrappers/ISAMServerAccessor.cs (1)
1-27: LGTM!Clean abstraction that enables dependency injection and testability. The interface and implementation correctly delegate to
SAMMethods.SamConnectwith appropriate error handling via theResultpattern.src/CommonLib/Processors/LocalGroupProcessor.cs (1)
45-54: LGTM!The method correctly delegates to the accessor and maintains the existing result wrapping logic.
src/CommonLib/Processors/CertAbuseProcessor.cs (7)
21-34: LGTM!Good dependency injection pattern that enables testability. The accessors are properly stored and the adaptive timeout loggers are appropriately named.
45-69: LGTM!Status reporting correctly covers both success and failure paths with appropriate task naming.
213-218: LGTM!Good defensive null check for
DiscretionaryAclbefore iterating.
258-278: LGTM!Registry access properly delegated to the injected accessor.
289-327: LGTM!Properly updated to async with status reporting on both paths. The signature change to accept
computerObjectIdaligns with the status reporting requirements.
469-535: LGTM!Good defensive coding with the null opaque check. The
AllTemplatespath and fallback template resolution logic are well-structured.
537-540: No action needed. The return type fromExecuteRPCWithTimeout(...).GetAwaiter().GetResult()is alreadySharpHoundRPC.Result<ISAMServer>, matching the method signature. The CertAbuseProcessor implementation is correct and compatible with LocalGroupProcessor. Both return the same type; LocalGroupProcessor merely re-wraps the result unnecessarily.test/unit/CertAbuseProcessorTest.cs (4)
35-47: LGTM!Good test setup with proper mock initialization, event subscription for status verification, and cache isolation via
SetCacheInstance.
49-169: LGTM!Comprehensive tests for
IsUserSpecifiesSanEnabledandIsRoleSeparationEnabledcovering both success and failure paths with proper status event verification.
229-278: LGTM!Good use of
MemberDatafor testingProcessEAPermissionswith different DACL scenarios (null and empty).
632-710: LGTM!Thorough coverage of
CreateEnrollmentAgentRestrictionedge cases including null opaque, unresolved templates, and theAllTemplatespath.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@test/unit/CertAbuseProcessorTest.cs`:
- Line 701: The test is passing nameof(Label.User) as the target parameter to
CreateEnrollmentAgentRestriction, which is inconsistent with similar tests that
use TargetName; update the call that uses emptyOpaqueAce so the third argument
(target) is TargetName instead of nameof(Label.User) to ensure the test
exercises the same code path as the other cases (look for the
CreateEnrollmentAgentRestriction invocation near the variables emptyOpaqueAce,
TargetDomainSid and sid and replace the target argument accordingly).
♻️ Duplicate comments (1)
test/unit/CertAbuseProcessorTest.cs (1)
625-629: Past review feedback addressed.Line 628 now correctly uses
CSVComputerStatus.StatusSuccess, which is consistent with other success path tests in the file. This addresses the inconsistency flagged in previous reviews.
🧹 Nitpick comments (2)
test/unit/CertAbuseProcessorTest.cs (2)
77-77: Inconsistentnameofusage pattern.Line 77 uses
nameof(CertAbuseProcessor.IsUserSpecifiesSanEnabled)(class name) while line 105 usesnameof(_certAbuseProcessor.IsUserSpecifiesSanEnabled)(instance). This pattern repeats throughout the file (e.g., lines 137 vs 166, 195 vs 224, 304 vs 333). While both resolve to the same string, consider using a consistent style—preferably the class name variant since it doesn't depend on an instance variable.Also applies to: 105-105
654-666: Misleading variable nameemptyOpaqueAce.The variable
emptyOpaqueAce(lines 654, 681, 717, 750) contains ACEs with populated opaque byte arrays, making the name misleading. Consider renaming to something more descriptive likeopaqueAceoraceWithRestrictionDatato improve test readability.Also applies to: 681-693
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/unit/CertAbuseProcessorTest.cs
⏰ 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 (5)
test/unit/CertAbuseProcessorTest.cs (5)
35-47: LGTM - Constructor and test setup are well-structured.The constructor properly initializes mocks, subscribes to the
ComputerStatusEventfor validation, and resets the cache for test isolation. This is a clean pattern for dependency-injected test classes.
110-169: LGTM - IsRoleSeparationEnabled tests are correct.The test coverage for both success and failure paths is appropriate. The mock setup and assertions properly validate both the method results and the computer status logging.
171-278: LGTM - ProcessEAPermissions tests provide good coverage.The test data provider at line 229 correctly covers both null and empty DACL edge cases. The Windows-only theory is appropriately used for platform-specific tests involving
RawAcl.
338-385: LGTM - ProcessRegistryEnrollmentPermissions tests are comprehensive.The Windows-only test properly sets up the security descriptor with no owner and no rules. The mock setup chain for registry, LDAP utils, and SAM server is correct.
387-537: LGTM - ProcessCertTemplates and GetRegistryPrincipal tests are thorough.The tests cover the key resolution paths including filtered SIDs, domain controller vs non-DC scenarios, local vs domain principal resolution. The
VerifyNoOtherCalls()assertions are a good practice to ensure no unexpected interactions.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
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.