ADCS enhancement #221
ADCS enhancement #221
Conversation
|
Warning Rate limit exceeded@JonasBK has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 12 minutes and 14 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThe changes introduce new constants and GUIDs for "WriteAltSecurityIdentities" and "WritePublicInformation" in several utility classes. ACL processing logic is extended to handle these new rights for user and computer objects. The certificate abuse processor gains methods to check RPC encryption enforcement and disabled extensions. Related data models and tests are updated accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant CertAbuseProcessor
participant Registry
Caller->>CertAbuseProcessor: RPCEncryptionEnforced(target, caName)
CertAbuseProcessor->>Registry: Read InterfaceFlags from CA config key
Registry-->>CertAbuseProcessor: Return InterfaceFlags value
CertAbuseProcessor-->>Caller: Return BoolRegistryAPIResult
Caller->>CertAbuseProcessor: DisabledExtensions(target, caName)
CertAbuseProcessor->>Registry: Read Active policy module from PolicyModules
Registry-->>CertAbuseProcessor: Return Active policy module name
CertAbuseProcessor->>Registry: Read DisableExtensionList from active policy subkey
Registry-->>CertAbuseProcessor: Return DisableExtensionList array
CertAbuseProcessor-->>Caller: Return StringArrayRegistryAPIResult
Poem
✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🔭 Outside diff range comments (1)
src/CommonLib/Processors/CertAbuseProcessor.cs (1)
254-293: Critical logic errors in IsUserSpecifiesSanEnabled method.The method has several critical issues:
Incorrect registry value name: The first registry call uses
subValue = "EditFlags"but should besubValue = "Active"to read the active policy name.Null reference logic error: Lines 268-270 attempt to cast
data.Valueto string when it's null, which will cause a null reference exception.Wrong variable reference: Line 289 uses
(int)data.Valuebut should use(int)data2.Valuesince we're processing the second registry call.Apply this diff to fix the issues:
public BoolRegistryAPIResult IsUserSpecifiesSanEnabled(string target, string caName) { var ret = new BoolRegistryAPIResult(); var activePolicy = "CertificateAuthority_MicrosoftDefault.Policy"; var subKey = $"SYSTEM\\CurrentControlSet\\Services\\CertSvc\\Configuration\\{caName}\\PolicyModules"; - const string subValue = "EditFlags"; + const string subValue = "Active"; var data = Helpers.GetRegistryKeyData(target, subKey, subValue, _log); ret.Collected = data.Collected; if (!data.Collected) { ret.FailureReason = data.FailureReason; return ret; } - if (data.Value == null) + if (data.Value != null) { activePolicy = (string)data.Value; } var subKey2 = $"SYSTEM\\CurrentControlSet\\Services\\CertSvc\\Configuration\\{caName}\\PolicyModules\\{activePolicy}"; const string subValue2 = "EditFlags"; var data2 = Helpers.GetRegistryKeyData(target, subKey2, subValue2, _log); ret.Collected = data2.Collected; if (!data2.Collected) { ret.FailureReason = data2.FailureReason; return ret; } if (data2.Value == null) { return ret; } - var editFlags = (int)data.Value; + var editFlags = (int)data2.Value; ret.Value = (editFlags & 0x00040000) == 0x00040000; return ret; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/CommonLib/Enums/EdgeNames.cs(1 hunks)src/CommonLib/OutputTypes/APIResults/StringArrayRegistryAPIResult.cs(1 hunks)src/CommonLib/OutputTypes/CARegistryData.cs(1 hunks)src/CommonLib/Processors/ACEGuids.cs(1 hunks)src/CommonLib/Processors/ACLProcessor.cs(1 hunks)src/CommonLib/Processors/CertAbuseProcessor.cs(3 hunks)test/unit/ACLProcessorTest.cs(59 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
src/CommonLib/OutputTypes/CARegistryData.cs (2)
src/CommonLib/OutputTypes/APIResults/BoolRegistryAPIResult.cs (1)
BoolRegistryAPIResult(3-6)src/CommonLib/OutputTypes/APIResults/StringArrayRegistryAPIResult.cs (1)
StringArrayRegistryAPIResult(5-8)
src/CommonLib/Processors/CertAbuseProcessor.cs (3)
src/CommonLib/Helpers.cs (1)
Helpers(16-313)src/CommonLib/OutputTypes/APIResults/BoolRegistryAPIResult.cs (1)
BoolRegistryAPIResult(3-6)src/CommonLib/OutputTypes/APIResults/StringArrayRegistryAPIResult.cs (1)
StringArrayRegistryAPIResult(5-8)
⏰ 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 (12)
src/CommonLib/OutputTypes/APIResults/StringArrayRegistryAPIResult.cs (1)
5-8: Well-structured registry API result class following established patterns.The implementation correctly inherits from
APIResultand provides aDataproperty with sensible initialization. The pattern matches other registry result classes in the codebase.src/CommonLib/OutputTypes/CARegistryData.cs (1)
11-12: Consistent addition of new CA registry properties.The new properties
RPCEncryptionEnforcedandDisabledExtensionsfollow the established pattern and use appropriate types that match the new registry API result classes.src/CommonLib/Processors/CertAbuseProcessor.cs (2)
296-320: Well-implemented RPCEncryptionEnforced method.The method correctly reads the
InterfaceFlagsregistry value and checks for the RPC encryption bit (0x00000200). The implementation follows the established pattern for registry access and error handling.
323-365: Correctly implemented DisabledExtensions method.The method properly implements the two-step registry access pattern:
- First reads the active policy name from the PolicyModules registry key
- Then reads the DisableExtensionList from the specific active policy subkey
The logic for handling null values and error cases is correct, following the established patterns in the codebase.
src/CommonLib/Enums/EdgeNames.cs (1)
25-26: LGTM! New edge name constants properly implemented.The new constants follow the established naming convention and are correctly positioned within the existing constant definitions. The names align with the corresponding GUIDs and processing logic in the other files.
src/CommonLib/Processors/ACLProcessor.cs (2)
621-629: LGTM! WriteAltSecurityIdentities processing correctly implemented.The conditional logic follows the established pattern and correctly handles User and Computer object types. The ACE object creation includes all required properties and uses the appropriate constants.
630-638: LGTM! WritePublicInformation processing correctly implemented.The conditional logic is consistent with the existing pattern and properly handles the new ACE type. The implementation maintains the same structure and property assignments as other ACE processing logic.
test/unit/ACLProcessorTest.cs (5)
21-22: LGTM: Consistent formatting improvementsThe addition of braces to class and method declarations improves code consistency and follows C# best practices.
Also applies to: 40-45, 47-49, 52-55
2095-2132: Excellent test coverage for new WriteAltSecurityIdentities ACE typeThis test method properly covers the new WriteAltSecurityIdentities permission for User objects. It follows the established testing patterns, uses appropriate mocking, and verifies all expected assertions.
2134-2171: Comprehensive coverage for WriteAltSecurityIdentities on Computer objectsThis test ensures the new WriteAltSecurityIdentities permission works correctly for Computer objects, providing complete coverage alongside the User test.
2173-2210: Well-implemented test for WritePublicInformation ACE typeThis test method thoroughly covers the new WritePublicInformation permission for User objects, following established patterns and ensuring proper functionality verification.
2212-2249: Complete test coverage for WritePublicInformation on Computer objectsThis test rounds out the comprehensive coverage for the new ACE types, ensuring WritePublicInformation works correctly for Computer objects. The implementation follows established patterns and completes the test matrix for the new functionality.
Description
Collection of two new ACL permissions:
Collection of CA registry key values:
Motivation and Context
Tickets: BED-6155, BED-6176
How Has This Been Tested?
Locally in lab environment.
Types of changes
Checklist:
Summary by CodeRabbit
New Features
Bug Fixes
Tests