Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds support for configuring key usages in certificate signing requests (CSRs) and certificates by introducing a KeyUsage enum and updating the CSR generation logic to properly handle key usage extensions.
Changes:
- Introduced a
KeyUsageenum to replace string-based key usage validation - Refactored CSR key usage handling to use a single
X509KeyUsageExtensioninstead of multiple extensions - Added
KeyUsageparameter toNew-SCEPmanCertificatefunction with proper parameter forwarding
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| SCEPmanClient/Private/x509/constants.ps1 | Defines new KeyUsage enum with valid key usage values |
| SCEPmanClient/Public/New-CSR.ps1 | Refactors key usage handling to use enum type and creates single extension with combined flags |
| SCEPmanClient/Public/New-SCEPmanCertificate.ps1 | Adds KeyUsage parameter and passes it to CSR creation |
| Tests/New-CSR.Tests.ps1 | Updates test to validate key usages are properly set in the CSR extension |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| It "should return a valid CSR with validatable key usages" { | ||
| $Request = New-CSR -PrivateKey (New-PrivateKey) -Subject "CN=Test" -ExtendedKeyUsage ClientAuth, ServerAuth -KeyUsage KeyEncipherment, DigitalSignature -Raw | ||
|
|
||
| $Request.CertificateExtensions.KeyUsages | Should -BeExactly 'KeyEncipherment, DigitalSignature' |
There was a problem hiding this comment.
The test validates key usages but doesn't verify that the extension is marked as critical, which is set to $true in line 135 of New-CSR.ps1. Consider adding an assertion to verify the Critical property of the key usage extension.
| $KeyUsageExtension = [System.Security.Cryptography.X509Certificates.X509KeyUsageExtension]::new( | ||
| [System.Security.Cryptography.X509Certificates.X509KeyUsageFlags] $KeyUsages, | ||
| $true # Critical | ||
| ) |
There was a problem hiding this comment.
The $KeyUsages variable contains string values (output from ToString() on line 121), but the cast to X509KeyUsageFlags expects enum values or their string representations that match the enum member names. The conversion may fail or produce unexpected results if the string format doesn't match exactly what the enum expects. Verify that the string-to-enum conversion works correctly, or consider using the original KeyUsage enum values directly instead of converting to strings first.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Add the option to add key usages in New-SCEPmanCertificate and change the way we add the KeyUsageExtension to the CSR to be considered when they are signed.