73288 iis orchestrator 2.6.3 #147
Conversation
…d properly quotes the RDN values containing escaped commas.
…_comma #ab73289 Resolved issues parsing Distinguished Name
64674 remove old certs on renewal
…ord_Complex #ab74699 - Fixed an error with complex PFX passwords
75552 iis binding issues
…//github.com/Keyfactor/iis-orchestrator into 75657-Update_Documentation_For_Container_Use
…_Container_Use 75657 update documentation for container use & Fixed Null error
…_null 76007 CSP formatted wrong when null
There was a problem hiding this comment.
Pull Request Overview
This PR updates the IIS orchestrator to version 2.6.3, introducing several bug fixes and improvements for certificate management operations. The changes primarily focus on fixing issues with PFX certificate passwords, improving certificate store management logic, and enhancing CSP reporting capabilities.
- Fixed error handling for PFX certificates with complex passwords containing special characters
- Updated certificate renewal logic to automatically remove unused certificates from stores
- Improved CSP detection for certificates using newer CNG keys
Reviewed Changes
Copilot reviewed 12 out of 24 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| integration-manifest.json | Updated CSP parameter descriptions to reference new documentation section |
| docsource/content.md | Added comprehensive CSP documentation and Docker container support instructions |
| README.md | Enhanced documentation with CSP usage guidelines and container requirements |
| IISU/PowerShellScripts/WinCertScripts.ps1 | Major refactoring of certificate management functions with improved error handling |
| IISU/PSHelper.cs | Added null safety check for PowerShell parameters |
| IISU/ImplementedStoreTypes/WinIIS/Management.cs | Added automatic removal of unused certificates during renewals |
| IISU/ImplementedStoreTypes/*/Inventory.cs | Enhanced inventory reporting with item counts |
| IISU/ClientPSCertStoreReEnrollment.cs | Added handling for None binding type in re-enrollment |
| CHANGELOG.md | Documented all bug fixes and improvements in version 2.6.3 |
| .github/workflows/keyfactor-starter-workflow.yml | Updated workflow to version 4 with additional configuration |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| # Get the thumbprint of the passed in certificate | ||
| # Convert password to secure string if provided, otherwise use $null | ||
| $bytes = [System.Convert]::FromBase64String($Base64Cert) | ||
| $securePassword = if ($PrivateKeyPassword) { ConvertTo-SecureString -String $PrivateKeyPassword -AsPlainText -Force } else { $null } | ||
|
|
||
| # Set the storage flags and get the certificate's thumbprint | ||
| $keyStorageFlags = [System.Security.Cryptography.X509Certificates.X509KeyStorageFlags]::PersistKeySet -bor ` | ||
| [System.Security.Cryptography.X509Certificates.X509KeyStorageFlags]::MachineKeySet | ||
|
|
||
| $cert = New-Object System.Security.Cryptography.X509Certificates.X509Certificate2($bytes, $securePassword, $keyStorageFlags) | ||
| $thumbprint = $cert.Thumbprint | ||
|
|
||
| if (-not $thumbprint) { throw "Failed to get the certificate thumbprint. The PFX may be invalid or the password is incorrect." } |
There was a problem hiding this comment.
The certificate object is created twice in this function - once here to get the thumbprint and again later in the 'else' block (line 310). This creates unnecessary duplication and potential for inconsistency. Consider extracting the certificate creation logic into a single location and reusing the certificate object.
Copilot uses AI. Check for mistakes.
| # Quote any arguments with spaces | ||
| $argLine = ($arguments | ForEach-Object { | ||
| if ($_ -match '\s') { '"{0}"' -f $_ } else { $_ } | ||
| }) -join ' ' | ||
|
|
||
| $cert = New-Object -TypeName System.Security.Cryptography.X509Certificates.X509Certificate2 -ArgumentList $bytes, $PrivateKeyPassword, 18 <# Persist, Machine #> | ||
| $certStore.Add($cert) | ||
| $certStore.Close(); | ||
| Write-Information "Store '$StoreName' is closed." | ||
| write-Verbose "Running certutil with arguments: $argLine" | ||
|
|
||
| # Get the thumbprint so it can be returned to the calling function | ||
| $thumbprint = $cert.Thumbprint | ||
| Write-Information "The thumbprint '$thumbprint' was created." | ||
| } | ||
| # Setup process execution | ||
| $processInfo = New-Object System.Diagnostics.ProcessStartInfo | ||
| $processInfo.FileName = "certutil.exe" | ||
| $processInfo.Arguments = $argLine.Trim() | ||
| $processInfo.RedirectStandardOutput = $true | ||
| $processInfo.RedirectStandardError = $true | ||
| $processInfo.UseShellExecute = $false | ||
| $processInfo.CreateNoWindow = $true |
There was a problem hiding this comment.
The argument quoting logic only handles spaces but doesn't properly escape other shell metacharacters that could lead to command injection. Consider using proper shell escaping or ProcessStartInfo.ArgumentList property to pass arguments securely instead of building a command line string.
Copilot uses AI. Check for mistakes.
| [NewRequest] | ||
| Subject = "$SubjectText" | ||
| Subject = "$parsedSubject" |
There was a problem hiding this comment.
[nitpick] The variable $parsedSubject is used without proper string escaping for PowerShell here-strings. While the Parse-DNSubject function handles quote escaping, this direct substitution could still be fragile. Consider using a more explicit approach like -f formatting or ensuring the Parse-DNSubject function returns properly escaped values for here-string contexts.
Copilot uses AI. Check for mistakes.
| if ($cngKey -and $cngKey.Provider -and $cngKey.Provider.Provider) { | ||
| return [string]$cngKey.Provider.Provider |
There was a problem hiding this comment.
The nested property access chain $cngKey.Provider.Provider could throw null reference exceptions if any intermediate property returns null. Consider using the null-conditional operator or explicit null checks for each level to prevent runtime errors.
| if ($cngKey -and $cngKey.Provider -and $cngKey.Provider.Provider) { | |
| return [string]$cngKey.Provider.Provider | |
| if ($cngKey) { | |
| if ($cngKey.Provider) { | |
| if ($cngKey.Provider.Provider) { | |
| return [string]$cngKey.Provider.Provider | |
| } | |
| } |
Copilot uses AI. Check for mistakes.
| } | ||
|
|
||
| try { | ||
| $key = [System.Security.Cryptography.X509Certificates.RSACertificateExtensions]::GetRSAPrivateKey($cert) |
There was a problem hiding this comment.
The variable name $cert should be $Cert (with capital C) to match the parameter name defined on line 1285. This inconsistency could cause the function to fail at runtime.
| $key = [System.Security.Cryptography.X509Certificates.RSACertificateExtensions]::GetRSAPrivateKey($cert) | |
| $key = [System.Security.Cryptography.X509Certificates.RSACertificateExtensions]::GetRSAPrivateKey($Cert) |
Copilot uses AI. Check for mistakes.
Fixed error when adding PFX Cert with complex password.
Updated logic to remove old, unused certs upon a renewal.
Fixed bug on ODKG jobs when org names contains a coma in the name.
Fixed an issue reporting CSPs using newer CNG keys.