Add DisabledExtensions CA reg key #236
Conversation
WalkthroughA new API result class for returning string arrays was introduced. The Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant CertAbuseProcessor
participant Registry
Caller->>CertAbuseProcessor: IsUserSpecifiesSanEnabled(target, caName)
CertAbuseProcessor->>Registry: Read "Active" from PolicyModules
Registry-->>CertAbuseProcessor: Return activePolicyName
CertAbuseProcessor->>Registry: Read "EditFlags" from activePolicyName subkey
Registry-->>CertAbuseProcessor: Return EditFlags value
CertAbuseProcessor-->>Caller: Return BoolRegistryAPIResult
Caller->>CertAbuseProcessor: DisabledExtensions(target, caName)
CertAbuseProcessor->>Registry: Read "Active" from PolicyModules
Registry-->>CertAbuseProcessor: Return activePolicyName
CertAbuseProcessor->>Registry: Read "DisableExtensionList" from activePolicyName subkey
Registry-->>CertAbuseProcessor: Return string[] of disabled extensions
CertAbuseProcessor-->>Caller: Return StringArrayRegistryAPIResult
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches
🧪 Generate unit tests
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. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
WalkthroughA new API result class for string arrays was introduced. The Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant CertAbuseProcessor
participant Registry
Caller->>CertAbuseProcessor: DisabledExtensions(target, caName)
CertAbuseProcessor->>Registry: Read "Active" value from PolicyModules
alt "Active" value exists
CertAbuseProcessor->>Registry: Read "DisableExtensionList" from active policy subkey
Registry-->>CertAbuseProcessor: Return string array or error
else No "Active" value
CertAbuseProcessor->>Registry: Use default policy subkey, read "DisableExtensionList"
Registry-->>CertAbuseProcessor: Return string array or error
end
CertAbuseProcessor-->>Caller: Return StringArrayRegistryAPIResult
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches
🧪 Generate unit tests
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. 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: 0
🧹 Nitpick comments (1)
src/CommonLib/Processors/CertAbuseProcessor.cs (1)
298-341: LGTM! New method follows established patterns.The
DisabledExtensionsmethod is well-implemented and follows the same registry querying pattern as other methods in the class.Consider extracting the common active policy resolution logic into a helper method to reduce code duplication between
IsUserSpecifiesSanEnabledandDisabledExtensions:+private RegistryResult GetActivePolicy(string target, string caName) +{ + var subKey = $"SYSTEM\\CurrentControlSet\\Services\\CertSvc\\Configuration\\{caName}\\PolicyModules"; + const string subValue = "Active"; + return Helpers.GetRegistryKeyData(target, subKey, subValue, _log); +}Then both methods could use this helper to get the active policy name before querying specific values.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/CommonLib/OutputTypes/APIResults/StringArrayRegistryAPIResult.cs(1 hunks)src/CommonLib/OutputTypes/CARegistryData.cs(1 hunks)src/CommonLib/Processors/CertAbuseProcessor.cs(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/CommonLib/OutputTypes/CARegistryData.cs (1)
src/CommonLib/OutputTypes/APIResults/StringArrayRegistryAPIResult.cs (1)
StringArrayRegistryAPIResult(5-8)
🔇 Additional comments (3)
src/CommonLib/OutputTypes/APIResults/StringArrayRegistryAPIResult.cs (1)
1-9: LGTM! Clean and consistent implementation.The class follows established patterns and uses
Array.Empty<String>()for efficient default initialization.src/CommonLib/OutputTypes/CARegistryData.cs (1)
11-11: LGTM! Property added consistently.The new
DisabledExtensionsproperty follows the established pattern and naming conventions of the class.src/CommonLib/Processors/CertAbuseProcessor.cs (1)
254-296: LGTM! Dynamic policy resolution improves robustness.The updated method now dynamically determines the active policy instead of using a hardcoded value, which makes it more robust for different CA configurations.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/CommonLib/Processors/CertAbuseProcessor.cs (4)
257-341: Consider extracting common policy retrieval logic.Both
IsUserSpecifiesSanEnabledandDisabledExtensionsmethods share identical logic for retrieving the active policy name. Consider extracting this into a private helper method to reduce code duplication and improve maintainability.+private (bool Collected, string FailureReason, string ActivePolicy) GetActivePolicy(string target, string caName) +{ + var activePolicy = "CertificateAuthority_MicrosoftDefault.Policy"; + var subKey = $"SYSTEM\\CurrentControlSet\\Services\\CertSvc\\Configuration\\{caName}\\PolicyModules"; + const string subValue = "Active"; + var data = Helpers.GetRegistryKeyData(target, subKey, subValue, _log); + + if (!data.Collected) + { + return (false, data.FailureReason, null); + } + + if (data.Value != null) + { + activePolicy = (string)data.Value; + } + + return (true, null, activePolicy); +}Then both methods can use this helper to reduce duplication.
259-259: Consider using constants for registry paths.The registry key paths are constructed inline in multiple places. Consider defining constants for the base paths to improve maintainability and reduce the risk of typos.
+private const string CERT_SVC_CONFIG_BASE = "SYSTEM\\CurrentControlSet\\Services\\CertSvc\\Configuration"; +private const string POLICY_MODULES_SUBPATH = "PolicyModules";Then use these constants when constructing the full paths.
Also applies to: 276-276, 304-304, 321-321
298-341: Method implementation is correct but consider extracting common logic.The
DisabledExtensionsmethod follows the same pattern asIsUserSpecifiesSanEnabledwith proper error handling and registry access. However, there's significant code duplication between these two methods.Consider extracting the common policy retrieval logic into a helper method:
+ private (bool Success, string ActivePolicy, string FailureReason) GetActivePolicy(string target, string caName) + { + var activePolicy = "CertificateAuthority_MicrosoftDefault.Policy"; + var subKey = $"SYSTEM\\CurrentControlSet\\Services\\CertSvc\\Configuration\\{caName}\\PolicyModules"; + const string subValue = "Active"; + var data = Helpers.GetRegistryKeyData(target, subKey, subValue, _log); + + if (!data.Collected) + { + return (false, null, data.FailureReason); + } + + if (data.Value != null) + { + activePolicy = (string)data.Value; + } + + return (true, activePolicy, null); + }This would eliminate the duplicated code in both methods and improve maintainability.
257-341: Consider extracting common policy resolution logic.Both
IsUserSpecifiesSanEnabledandDisabledExtensionsmethods contain duplicate code for determining the active policy. Consider extracting this into a helper method to improve maintainability.Example refactor:
+private (bool Success, string ActivePolicy, string FailureReason) GetActivePolicy(string target, string caName) +{ + var activePolicy = "CertificateAuthority_MicrosoftDefault.Policy"; + var subKey = $"SYSTEM\\CurrentControlSet\\Services\\CertSvc\\Configuration\\{caName}\\PolicyModules"; + const string subValue = "Active"; + var data = Helpers.GetRegistryKeyData(target, subKey, subValue, _log); + + if (!data.Collected) + { + return (false, null, data.FailureReason); + } + + if (data.Value != null) + { + activePolicy = (string)data.Value; + } + + return (true, activePolicy, null); +}Then both methods can use this helper to reduce duplication.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/CommonLib/OutputTypes/APIResults/StringArrayRegistryAPIResult.cs(1 hunks)src/CommonLib/OutputTypes/CARegistryData.cs(1 hunks)src/CommonLib/Processors/CertAbuseProcessor.cs(3 hunks)
🔇 Additional comments (11)
src/CommonLib/OutputTypes/APIResults/StringArrayRegistryAPIResult.cs (3)
1-9: LGTM! Clean and well-structured implementation.The new
StringArrayRegistryAPIResultclass follows C# best practices with proper inheritance, naming conventions, and safe default initialization. The class serves its intended purpose as a container for string array registry results.
1-9: LGTM! Clean and focused API result implementation.The class follows the established pattern for API result types in the codebase. The initialization to
Array.Empty<String>()is appropriate and memory-efficient compared tonew string[0].
1-9: LGTM! Clean and efficient implementation.The new
StringArrayRegistryAPIResultclass follows established patterns, usesArray.Empty<String>()for optimal performance, and provides a focused container for string array registry results.src/CommonLib/OutputTypes/CARegistryData.cs (3)
11-11: LGTM! Consistent integration of the new property.The
DisabledExtensionsproperty follows the established pattern of theCARegistryDataclass and uses the appropriateStringArrayRegistryAPIResulttype for holding string array registry data.
11-11: LGTM! Property addition follows established patterns.The new
DisabledExtensionsproperty is consistent with the existing properties in the class, using the appropriateStringArrayRegistryAPIResulttype and following the same naming conventions.
11-11: LGTM! Property addition follows established patterns.The new
DisabledExtensionsproperty is consistent with other registry result properties in the class and uses the appropriate specialized result type.src/CommonLib/Processors/CertAbuseProcessor.cs (5)
257-296: Excellent refactoring to use dynamic policy detection.The refactoring from hardcoded policy name to dynamically reading the "Active" policy value makes the code more robust and flexible. The improved error handling with proper failure reason propagation is also a good enhancement.
298-341: Well-implemented new method with consistent pattern.The
DisabledExtensionsmethod follows the same robust pattern as the refactoredIsUserSpecifiesSanEnabledmethod, with proper error handling and dynamic policy detection. The string array casting and return type are appropriate for the DisableExtensionList registry value.
257-296: Good refactoring to use dynamic policy determination.The method now correctly reads the active policy from the registry instead of using a hardcoded value. The error handling has been improved to properly propagate failure reasons from both registry reads.
257-296: LGTM! Improved dynamic policy resolution.The refactoring to dynamically determine the active policy subkey instead of using a hardcoded value is a significant improvement. The error handling is properly implemented with failure reason propagation.
298-341: LGTM! New method follows established patterns.The new
DisabledExtensionsmethod correctly implements the registry access pattern and properly handles error cases. The implementation aligns well with the refactoredIsUserSpecifiesSanEnabledmethod.
1 participant
Description
Collection of CA registry key DisabledExtensions for ADCS ESC16 edge
Also updates the collection of CA reg key setting
IsUserSpecifiesSanEnabledto support custom reg key paths.Motivation and Context
Tickets: BED-6176
How Has This Been Tested?
Locally in lab environment.
Types of changes
Checklist:
Summary by CodeRabbit