Collect EnrollmentAgent Restrictions For AllTemplates, add CertAbuseProcessor Tests BED-7044 #268
Conversation
WalkthroughThis PR refactors registry and SAM server access by extracting direct utility methods from Helpers.cs and SHRegistryKey into new dependency-injectable IRegistryAccessor and ISAMServerAccessor interfaces with concrete implementations (RegistryAccessor and SAMServerAccessor). Processors are updated to use these accessors via constructor injection instead of calling static utility methods directly. Changes
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
🤖 Fix all issues with AI agents
In @test/unit/CertAbuseProcessorTest.cs:
- Line 701: The test passes an incorrect value for the computerDomain parameter
to CreateEnrollmentAgentRestriction: it uses nameof(Label.User) ("User") instead
of the domain string; update the call in the test to pass DomainName (e.g.,
"TEST.LOCAL") as the computerDomain argument so CreateEnrollmentAgentRestriction
receives a valid domain consistent with other tests.
🧹 Nitpick comments (2)
src/CommonLib/Processors/ComputerSessionProcessor.cs (1)
27-27: Pass the logger to RegistryAccessor for consistent logging.
DCRegistryProcessorpasses the logger toRegistryAccessor(log), but here the logger is omitted. This causes logging inconsistency across processors.Additionally, consider accepting
IRegistryAccessoras an optional constructor parameter (defaulting tonew RegistryAccessor(_log)) to enable unit testing with mocks.♻️ Suggested improvement
- 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(_log); }Also applies to: 42-42
src/CommonLib/IRegistryAccessor.cs (1)
59-61: Consider adding a synchronous overload or documenting blocking behavior.
OpenRemoteRegistryblocks synchronously viaGetAwaiter().GetResult(). While this works, it could cause deadlocks if called from certain synchronization contexts (e.g., UI threads or ASP.NET classic). Since this is a Windows-specific library primarily used in console applications, this is likely acceptable, but consider documenting this behavior.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
src/CommonLib/Helpers.cssrc/CommonLib/IRegistryAccessor.cssrc/CommonLib/IRegistryKey.cssrc/CommonLib/Processors/CertAbuseProcessor.cssrc/CommonLib/Processors/ComputerSessionProcessor.cssrc/CommonLib/Processors/DCRegistryProcessor.cssrc/CommonLib/Processors/LocalGroupProcessor.cssrc/SharpHoundRPC/SAMRPCNative/SAMMethods.cssrc/SharpHoundRPC/Wrappers/ISAMServerAccessor.cssrc/SharpHoundRPC/Wrappers/SAMServer.cstest/unit/CertAbuseProcessorTest.cstest/unit/Facades/MockLdapUtils.cstest/unit/Utils.cs
💤 Files with no reviewable changes (2)
- src/SharpHoundRPC/Wrappers/SAMServer.cs
- src/SharpHoundRPC/SAMRPCNative/SAMMethods.cs
🧰 Additional context used
🧠 Learnings (4)
📚 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/Processors/DCRegistryProcessor.cssrc/CommonLib/IRegistryKey.cs
📚 Learning: 2025-06-26T16:37:09.005Z
Learnt from: definitelynotagoblin
Repo: SpecterOps/SharpHoundCommon PR: 217
File: src/CommonLib/Timeout.cs:21-21
Timestamp: 2025-06-26T16:37:09.005Z
Learning: In SharpHoundCommonLib's Timeout.cs, the team prefers to use TaskScheduler.Current instead of TaskScheduler.Default in Task.Factory.StartNew calls to maintain flexibility for custom schedulers, even though this requires awareness of potential deadlock risks in certain contexts.
Applied to files:
src/CommonLib/Processors/DCRegistryProcessor.cs
📚 Learning: 2025-06-26T16:38:49.677Z
Learnt from: definitelynotagoblin
Repo: SpecterOps/SharpHoundCommon PR: 217
File: src/CommonLib/Timeout.cs:17-36
Timestamp: 2025-06-26T16:38:49.677Z
Learning: In SharpHoundCommonLib's Timeout.cs, the team initially had concerns about disposing CancellationTokenSource instances before tasks could check cancellation tokens, but they understand that orphaned tasks (those that exceed timeout) won't be impacted by disposed tokens since their results are already ignored by the timeout handler. They prefer proper resource management with using statements for CancellationTokenSource disposal.
Applied to files:
src/CommonLib/Processors/DCRegistryProcessor.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/DCRegistryProcessor.cssrc/CommonLib/Processors/CertAbuseProcessor.cs
🧬 Code graph analysis (8)
test/unit/Utils.cs (1)
test/unit/CertAbuseProcessorTest.cs (1)
WindowsOnlyTheory(237-278)
src/CommonLib/Processors/LocalGroupProcessor.cs (1)
src/SharpHoundRPC/Wrappers/ISAMServerAccessor.cs (1)
SAMServerAccessor(13-26)
src/CommonLib/Processors/DCRegistryProcessor.cs (1)
src/CommonLib/IRegistryAccessor.cs (4)
Task(13-13)Task(75-80)RegistryAccessor(16-81)RegistryAccessor(20-23)
src/CommonLib/IRegistryAccessor.cs (1)
src/CommonLib/IRegistryKey.cs (2)
GetValue(6-6)GetValue(17-20)
src/SharpHoundRPC/Wrappers/ISAMServerAccessor.cs (3)
src/CommonLib/Processors/CertAbuseProcessor.cs (1)
SharpHoundRPC(537-540)src/CommonLib/Processors/LocalGroupProcessor.cs (1)
SharpHoundRPC(45-54)src/SharpHoundRPC/SAMRPCNative/SAMEnums.cs (1)
SAMEnums(6-74)
test/unit/CertAbuseProcessorTest.cs (8)
src/CommonLib/Processors/CertAbuseProcessor.cs (12)
SharpHoundRPC(537-540)CertAbuseProcessor(15-546)CertAbuseProcessor(27-34)Task(45-169)Task(179-232)Task(234-250)Task(289-327)Task(339-375)Task(377-410)Task(412-467)Task(469-535)Task(542-545)test/unit/LDAPUtilsTest.cs (7)
Fact(40-43)Fact(47-53)Fact(55-59)Fact(61-68)Fact(70-79)Fact(81-86)WindowsOnlyFact(261-287)src/CommonLib/LdapQueries/LdapFilter.cs (1)
IEnumerable(262-275)test/unit/Utils.cs (4)
WindowsOnlyTheory(94-100)WindowsOnlyTheory(96-99)WindowsOnlyFact(86-92)WindowsOnlyFact(88-91)src/SharpHoundRPC/Wrappers/SAMServer.cs (8)
Result(25-33)Result(35-42)Result(44-74)Result(76-90)Result(92-115)Result(117-131)SAMServer(12-138)SAMServer(17-21)src/CommonLib/OutputTypes/CertTemplate.cs (1)
CertTemplate(5-7)test/unit/WellKnownPrincipalTest.cs (1)
sid(48-117)src/CommonLib/OutputTypes/LocalGroup.cs (1)
LocalGroup(3-8)
src/CommonLib/Processors/CertAbuseProcessor.cs (3)
src/CommonLib/CSVComputerStatus.cs (1)
CSVComputerStatus(6-47)src/SharpHoundRPC/Wrappers/ISAMServerAccessor.cs (2)
Result(7-10)Result(15-25)src/SharpHoundRPC/Wrappers/SAMServer.cs (6)
Result(25-33)Result(35-42)Result(44-74)Result(76-90)Result(92-115)Result(117-131)
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)
🔇 Additional comments (26)
test/unit/Utils.cs (1)
94-100: LGTM!The
WindowsOnlyTheoryattribute correctly mirrors the existingWindowsOnlyFactpattern for parameterized[Theory]tests. The implementation is consistent with the established convention, and the relevant code snippet confirms it's already being used correctly inCertAbuseProcessorTest.cs.test/unit/Facades/MockLdapUtils.cs (1)
747-747: LGTM!Minor formatting adjustment with no functional impact.
src/CommonLib/Processors/ComputerSessionProcessor.cs (1)
298-298: LGTM!The registry access is now correctly routed through the
IRegistryAccessorabstraction, aligning with the PR's goal of centralizing remote registry access.src/CommonLib/Processors/DCRegistryProcessor.cs (2)
11-21: LGTM!The
RegistryAccessoris correctly instantiated with the logger, and the constructor properly initializes all dependencies. The refactoring successfully replaces directHelpers.GetRegistryKeyDatacalls with the new abstraction.
35-35: LGTM!All three registry access calls are correctly updated to use
_registryAccessor.GetRegistryKeyData, maintaining the existing method signatures and return value handling.Also applies to: 67-67, 99-99
src/CommonLib/Helpers.cs (1)
149-156: LGTM!Minor documentation comment formatting. The registry-related methods have been appropriately moved to
RegistryAccessoras part of the refactoring.src/CommonLib/Processors/LocalGroupProcessor.cs (3)
19-19: LGTM!The
ISAMServerAccessorabstraction is correctly integrated, enabling potential future testability improvements. The instantiation follows the established pattern in this PR.Also applies to: 31-31
34-34: LGTM!The logger name correctly reflects the new interface method
ISAMServerAccessor.OpenServer.
45-54: LGTM!The
OpenSamServermethod correctly delegates to_samServerAccessor.OpenServerand properly handles the result conversion.src/SharpHoundRPC/Wrappers/ISAMServerAccessor.cs (1)
1-27: LGTM! Clean abstraction for SAM server access.The interface and implementation correctly abstract the SAM server connection logic, enabling dependency injection and testability. The default access masks are appropriate for the enumeration and lookup operations used by consumers (CertAbuseProcessor, LocalGroupProcessor).
src/CommonLib/IRegistryKey.cs (1)
5-27: LGTM! Proper IDisposable implementation.The addition of
IDisposabletoIRegistryKeyand its implementation inSHRegistryKeycorrectly ensures the underlyingRegistryKeyis disposed. The public constructor enables the newRegistryAccessorto create instances. This aligns well with the using statement inRegistryAccessor.GetRegistryKeyData.test/unit/CertAbuseProcessorTest.cs (7)
35-47: Good test setup with proper isolation.The constructor correctly initializes mocks, creates the processor with injected dependencies, subscribes to the status event for verification, and resets the cache to ensure test isolation.
49-169: Comprehensive tests for registry-based flag checks.Good coverage of
IsUserSpecifiesSanEnabledandIsRoleSeparationEnabledwith both successful lookups (parameterized values) and failure handling. The assertions correctly validate both the return values and the status event reporting.
171-278: Good edge case coverage for ProcessEAPermissions.The tests cover empty registry values, failed lookups, and Windows-specific scenarios with null and empty DACLs. The
MemberDatapattern for parameterized tests is well-used.
338-385: LGTM! Test correctly validates empty result when no owner and no rules.The test properly sets up a security descriptor with no owner and configures the mock to return empty access rules, validating that the processor handles this edge case gracefully.
408-537: Excellent coverage of GetRegistryPrincipal branching logic.The tests thoroughly exercise each code path: filtered SIDs, domain controller resolution, well-known principal conversion, local SID resolution, and domain principal lookup. The
VerifyNoOtherCalls()assertions ensure no unexpected LDAP calls are made.
632-779: Good coverage of CreateEnrollmentAgentRestriction scenarios.The tests properly validate: null opaque data handling, unresolved template failure, all-templates path when no template is specified, and both canonical name and OID-based template resolution. The byte array constructions for opaque ACE data are well-commented.
628-628: The constants are equivalent and the assertion is correct.ComputerStatus.Successis a static string property that returns"Success", andCSVComputerStatus.StatusSuccessis a const string also set to"Success". Both resolve to the same value, so the test assertion at line 628 is valid.src/CommonLib/IRegistryAccessor.cs (3)
10-14: Well-designed interface for registry access abstraction.The interface provides a clean API for registry operations with clear method signatures. This enables dependency injection and testability for processors that need registry access.
25-57: Robust exception handling for registry operations.The method correctly handles the common exceptions that can occur during remote registry access:
IOException(connectivity),SecurityExceptionandUnauthorizedAccessException(permissions), with a catch-all for unexpected errors. Theusingstatement ensures proper disposal of the registry key.
75-80: Good use of AdaptiveTimeout for resilient registry connections.The method properly wraps the registry connection in an adaptive timeout and throws a documented
TimeoutExceptionon failure. The error message includes the machine name and error details for diagnostics.src/CommonLib/Processors/CertAbuseProcessor.cs (5)
21-34: Good dependency injection setup.The constructor properly accepts and stores the new
IRegistryAccessorandISAMServerAccessordependencies, enabling testability. The logger names for adaptive timeouts correctly reflect the operations they wrap.
53-69: Good addition of status reporting.The status reporting on both failure and success paths enables operational monitoring of registry enrollment permission processing. Using
nameof()for task names ensures consistency with method renames.
214-217: Important null guard for DiscretionaryAcl.This check prevents a
NullReferenceExceptionwhen iterating overdescriptor.DiscretionaryAclon line 220. A security descriptor can have a null DACL (which means allow all access), so returning early with empty restrictions is the correct behavior.
469-535: Well-structured enrollment agent restriction parsing.The method correctly:
- Guards against null opaque data
- Returns early with
AllTemplates=truewhen no template is specified (indicated byindex >= opaque.Length)- Falls back from canonical name to OID resolution for template lookup
- Returns failure when template cannot be resolved
The
-2offset in the Unicode string extraction (line 509) correctly accounts for the 2-byte null terminator.
537-540: OpenSamServer correctly delegates to accessor.The method properly wraps the accessor call with the adaptive timeout for resilience against slow or unresponsive servers.
| _mockLdapUtils.Setup(x => x.ResolveIDAndType("S-1-3", It.IsAny<string>())) | ||
| .ReturnsAsync((true, new TypedPrincipal("S-1-3", Label.User))); | ||
|
|
||
| var result = await _certAbuseProcessor.CreateEnrollmentAgentRestriction(emptyOpaqueAce, nameof(Label.User), DomainName, false, TargetDomainSid, sid); |
There was a problem hiding this comment.
Incorrect parameter value for computerDomain.
nameof(Label.User) evaluates to "User", which is passed as the computerDomain parameter. This should likely be DomainName (i.e., "TEST.LOCAL") to be consistent with other tests and represent a valid domain.
Suggested fix
- var result = await _certAbuseProcessor.CreateEnrollmentAgentRestriction(emptyOpaqueAce, nameof(Label.User), DomainName, false, TargetDomainSid, sid);
+ var result = await _certAbuseProcessor.CreateEnrollmentAgentRestriction(emptyOpaqueAce, DomainName, TargetName, false, TargetDomainSid, sid);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var result = await _certAbuseProcessor.CreateEnrollmentAgentRestriction(emptyOpaqueAce, nameof(Label.User), DomainName, false, TargetDomainSid, sid); | |
| var result = await _certAbuseProcessor.CreateEnrollmentAgentRestriction(emptyOpaqueAce, DomainName, TargetName, false, TargetDomainSid, sid); |
🤖 Prompt for AI Agents
In @test/unit/CertAbuseProcessorTest.cs at line 701, The test passes an
incorrect value for the computerDomain parameter to
CreateEnrollmentAgentRestriction: it uses nameof(Label.User) ("User") instead of
the domain string; update the call in the test to pass DomainName (e.g.,
"TEST.LOCAL") as the computerDomain argument so CreateEnrollmentAgentRestriction
receives a valid domain consistent with other tests.
Description
Collect EnrollmentAgent Restrictions For AllTemplates, add CertAbuseProcessor Tests
Motivation and Context
BED-7044
How Has This Been Tested?
Ran tests.
Verified that EnrollmentAgentRestriction was collected when CertificateTemplates is set to
All.Screenshots (if appropriate):
Types of changes
Checklist:
Summary by CodeRabbit
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.