81018 adding jea support#185
Open
rcpokorny wants to merge 58 commits into
Open
Conversation
…or/iis-orchestrator into 81018-Adding_JEA_Support
Contributor
There was a problem hiding this comment.
Pull request overview
This PR introduces JEA (Just Enough Administration) support for WinRM-based remote execution by refactoring the extension’s PowerShell surface into installable modules/role capabilities, wiring a new JEAEndpointName job property through the orchestrator, and updating tests/docs to cover the new behavior.
Changes:
- Add
JEAEndpointNamesupport end-to-end (job properties → PSHelper session creation → integration/unit tests). - Replace monolithic
WinCertScripts.ps1script injection with modular PowerShell (Keyfactor.WinCert.Common/IIS/SQL) plus JEA.psrcrole capability files and a.psscsession configuration template. - Update documentation (README + docsource) and integration tests to reflect JEA setup and connection selection.
Reviewed changes
Copilot reviewed 67 out of 68 changed files in this pull request and generated 17 comments.
Show a summary per file
| File | Description |
|---|---|
| WindowsCertStore.UnitTests/WindowsCertStore.UnitTests.csproj | Adds Moq/Newtonsoft.Json; sets xUnit VS runner PrivateAssets metadata. |
| WindowsCertStore.UnitTests/PSHelperUnitTests.cs | Updates script folder name lookup from PowerShellScripts to PowerShell. |
| WindowsCertStore.UnitTests/PSHelperConfigTests.cs | Adds unit tests for PSHelper constructors, script directory discovery, and LocalMachine+JEA guard behavior. |
| WindowsCertStore.UnitTests/JobPropertiesTests.cs | Adds unit tests validating JobProperties defaulting + legacy field tolerance. |
| WindowsCertStore.IntegrationTests/WinSQLIntegrationTests.cs | Adds Integration trait; passes JEAEndpointName; uses SQL-only connection data source. |
| WindowsCertStore.IntegrationTests/WinIISIntegrationTests.cs | Adds Integration trait; passes JEAEndpointName; uses IIS-only connection data source. |
| WindowsCertStore.IntegrationTests/WinCertIntegrationTests.cs | New WinCert E2E integration tests (Add/Inventory/Remove and renewal flow) with JEA support. |
| WindowsCertStore.IntegrationTests/servers.json | Expands server definitions with StoreType + JEA endpoint name. |
| WindowsCertStore.IntegrationTests/Factories/ConnectionFactory.cs | Refactors connection loading, adds store-type filtering and env/appsettings credential sourcing. |
| WindowsCertStore.IntegrationTests/ClientConnection.cs | Adds StoreType and JEAEndpointName to connection model. |
| README.md | Adds detailed JEA documentation and updates WinRM port references (5985/5986). |
| docsource/content.md | Adds detailed JEA documentation for generated docs; notes module install requirements for JEA. |
| IISU/WindowsCertStore.csproj | Updates packaging to include new PowerShell\ module tree + .pssc and legacy scripts. |
| IISU/RemoteSettings.cs | Adds JEAEndpointName to remote settings model. |
| IISU/PSHelper.cs | Adds JEA endpoint support (WinRM ConfigurationName), blocks LocalMachine+JEA, changes script loading strategy, improves Information stream handling. |
| IISU/Properties/AssemblyInfo.cs | Exposes internals to unit/integration test assemblies. |
| IISU/PowerShellScripts/WinCertScripts.ps1 | Removes legacy monolithic script. |
| IISU/PowerShell/WinADFSScripts.ps1 | Adjusts logging to use Information stream “[VERBOSE] …” convention. |
| IISU/PowerShell/Keyfactor.WinCert.SQL/RoleCapabilities/Keyfactor.WinCert.SQL.psrc | New SQL JEA role capability definition. |
| IISU/PowerShell/Keyfactor.WinCert.SQL/Public/Remove-KeyfactorSQLCertificate.ps1 | New SQL unbind function. |
| IISU/PowerShell/Keyfactor.WinCert.SQL/Public/New-KeyfactorSQLBinding.ps1 | New SQL bind function wrapper. |
| IISU/PowerShell/Keyfactor.WinCert.SQL/Public/Get-KeyfactorSQLInventory.ps1 | New SQL inventory function. |
| IISU/PowerShell/Keyfactor.WinCert.SQL/Private/Set-KeyfactorSQLCertificateBinding.ps1 | New SQL bind implementation (registry + ACL + optional restart). |
| IISU/PowerShell/Keyfactor.WinCert.SQL/Private/Get-KeyfactorSQLServiceUser.ps1 | New helper to read SQL service user. |
| IISU/PowerShell/Keyfactor.WinCert.SQL/Private/Get-KeyfactorSQLServiceName.ps1 | New helper to compute SQL service name. |
| IISU/PowerShell/Keyfactor.WinCert.SQL/Keyfactor.WinCert.SQL.psm1 | New SQL module entry point with explicit dot-sourcing + exports. |
| IISU/PowerShell/Keyfactor.WinCert.IIS/RoleCapabilities/Keyfactor.WinCert.IIS.psrc | New IIS JEA role capability definition. |
| IISU/PowerShell/Keyfactor.WinCert.IIS/Public/Remove-KeyfactorIISSiteBinding.ps1 | New IIS binding removal function with IIS drive autodetect. |
| IISU/PowerShell/Keyfactor.WinCert.IIS/Public/Remove-KeyfactorIISCertificateIfUnused.ps1 | New IIS certificate cleanup helper. |
| IISU/PowerShell/Keyfactor.WinCert.IIS/Public/New-KeyfactorIISSiteBinding.ps1 | New IIS binding creation with SSL flags handling and appcmd fallback. |
| IISU/PowerShell/Keyfactor.WinCert.IIS/Public/Get-KeyfactorIISBoundCertificates.ps1 | New IIS inventory function. |
| IISU/PowerShell/Keyfactor.WinCert.IIS/Private/Test-ValidSslFlags.ps1 | New SSL flag validation helper. |
| IISU/PowerShell/Keyfactor.WinCert.IIS/Private/Test-IISDrive.ps1 | New IIS:\ drive availability helper. |
| IISU/PowerShell/Keyfactor.WinCert.IIS/Private/Get-ValidSslFlagsForSystem.ps1 | New OS-build-based SSL flag support helper. |
| IISU/PowerShell/Keyfactor.WinCert.IIS/Private/Get-IISManagementInfo.ps1 | New IIS management mode detection helper. |
| IISU/PowerShell/Keyfactor.WinCert.IIS/Private/Add-IISBindingWithSSL.ps1 | New IIS binding add + SSL attach helper. |
| IISU/PowerShell/Keyfactor.WinCert.IIS/Keyfactor.WinCert.IIS.psm1 | New IIS module entry point with explicit dot-sourcing + exports. |
| IISU/PowerShell/Keyfactor.WinCert.Common/RoleCapabilities/Keyfactor.WinCert.Common.psrc | New common JEA role capability definition. |
| IISU/PowerShell/Keyfactor.WinCert.Common/Public/Remove-KeyfactorCertificate.ps1 | New common remove-from-store implementation. |
| IISU/PowerShell/Keyfactor.WinCert.Common/Public/New-KeyfactorResult.ps1 | New common structured result helper. |
| IISU/PowerShell/Keyfactor.WinCert.Common/Public/New-KeyfactorODKGEnrollment.ps1 | New ODKG CSR generation function. |
| IISU/PowerShell/Keyfactor.WinCert.Common/Public/Import-KeyfactorSignedCertificate.ps1 | New signed cert import helper. |
| IISU/PowerShell/Keyfactor.WinCert.Common/Public/Get-KeyfactorCertificates.ps1 | New common inventory function. |
| IISU/PowerShell/Keyfactor.WinCert.Common/Public/Add-KeyfactorCertificate.ps1 | New common add-to-store implementation. |
| IISU/PowerShell/Keyfactor.WinCert.Common/Private/Validate-CryptoProvider.ps1 | New provider validation helper. |
| IISU/PowerShell/Keyfactor.WinCert.Common/Private/Test-CryptoServiceProvider.ps1 | New provider existence helper. |
| IISU/PowerShell/Keyfactor.WinCert.Common/Private/Get-CryptoProviders.ps1 | New CSP discovery helper using certutil. |
| IISU/PowerShell/Keyfactor.WinCert.Common/Private/Get-CertificateSAN.ps1 | New SAN extraction helper. |
| IISU/PowerShell/Keyfactor.WinCert.Common/Private/Get-CertificateCSP.ps1 | New CSP/KSP detection helper (CAPI + CNG). |
| IISU/PowerShell/Keyfactor.WinCert.Common/Private/Convert-DNSSubject.ps1 | New DN subject parsing helper. |
| IISU/PowerShell/Keyfactor.WinCert.Common/Keyfactor.WinCert.Common.psm1 | New Common module entry point with explicit dot-sourcing + exports. |
| IISU/PowerShell/Build/KeyfactorWinCert.pssc | New JEA session configuration template (RestrictedRemoteServer + transcripts + role mapping). |
| IISU/Models/JobProperties.cs | Adds JEAEndpointName job property. |
| IISU/ImplementedStoreTypes/WinSQL/WinSqlBinding.cs | Switches to new SQL module function names. |
| IISU/ImplementedStoreTypes/WinSQL/Management.cs | Wires JEAEndpointName; switches to new module function names. |
| IISU/ImplementedStoreTypes/WinSQL/Inventory.cs | Wires JEAEndpointName; switches to new inventory function name and signature. |
| IISU/ImplementedStoreTypes/WinIIS/WinIISBinding.cs | Switches to new IIS module function names; updates unbind parameter shape and result parsing. |
| IISU/ImplementedStoreTypes/WinIIS/Management.cs | Wires JEAEndpointName; switches to new module function names. |
| IISU/ImplementedStoreTypes/WinIIS/Inventory.cs | Wires JEAEndpointName; switches to new IIS inventory function name. |
| IISU/ImplementedStoreTypes/WinADFS/Inventory.cs | Removes SAN field propagation for ADFS inventory parameters. |
| IISU/ImplementedStoreTypes/Win/WinInventory.cs | Marks unused class obsolete. |
| IISU/ImplementedStoreTypes/Win/WinCertCertificateInfo.cs | Removes SAN field from WinCert cert info model. |
| IISU/ImplementedStoreTypes/Win/Management.cs | Wires JEAEndpointName; switches to new module function names. |
| IISU/ImplementedStoreTypes/Win/Inventory.cs | Wires JEAEndpointName; switches to new inventory function name; removes SAN parameter propagation. |
| IISU/ClientPSCertStoreReEnrollment.cs | Wires JEAEndpointName; switches to new ODKG/import function names; updates log strings. |
| CHANGELOG.md | Adds 4.0.0 entry documenting SAN removal and JEA support. |
| .vscode/launch.json | Adds VS Code launch configuration for PowerShell. |
| .gitignore | Ignores local.runsettings to avoid committing local credentials. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+22
to
+29
| var config = new ConfigurationBuilder() | ||
| .SetBasePath(Directory.GetCurrentDirectory()) | ||
| .AddJsonFile("appsettings.json", optional: true) | ||
| .Build(); | ||
|
|
||
| // 3. Retrieve connection details from configuration | ||
| var vault = new VaultHelper(config); | ||
| return (vault.GetSecret("Username"), vault.GetSecret("Password")); | ||
| } |
|
|
||
| try { | ||
| Write-Information "Entering PowerShell Script Add-KeyfactorCertificateToStore" | ||
| Write-Information "[VERBOSE] Add-KeyfactorCertificateToStore - Received: StoreName: '$StoreName', CryptoServiceProvider: '$CryptoServiceProvider', Base64Cert: '$Base64Cert'" |
Comment on lines
+77
to
+78
| Write-Information "[VERBOSE] Running certutil with arguments: $argLine" | ||
|
|
Comment on lines
+10
to
+15
| if ($serviceUser) { | ||
| return $serviceUser | ||
| } else { | ||
| Write-Error "SQL Server instance '$SQLInstanceName' not found or no service user associated." | ||
| return $null | ||
| } |
| @@ -1,3 +1,7 @@ | |||
| 4.0.0 | |||
| * As of this version of the extension, SANs will be handled through the ODKG Enrollment page in Command and will no longer use the SAN Entry Parameter. This version, we are removing all support for the SAN Entry Parameter. If you are still using the SAN Entry Parameter, you will need to remove it from your store types and re-run inventory to remove it from your database. | |||
| * Adding JEA Support for local PowerShell execution. This will allow for more secure execution of the extension when running in a local PowerShell Runspace. To utilize this feature, you will need to create a JEA endpoint on the target server and specify the endpoint name as a new parameter in the specific Cert Store definition. Refer to the README for more details. | |||
Comment on lines
+28
to
+34
| Write-Information "[VERBOSE] Instance: $instance" | ||
| Write-Information "[VERBOSE] Full Instance: $fullInstance" | ||
| Write-Information "[VERBOSE] Registry Location: $regLocation" | ||
| Write-Information "[VERBOSE] Current Thumbprint: $currentThumbprint" | ||
|
|
||
| $currentThumbprint = Get-ItemPropertyValue -Path $regLocation -Name "Certificate" -ErrorAction SilentlyContinue | ||
|
|
Comment on lines
286
to
301
| _results = _psHelper.ExecutePowerShell("Add-KFCertificateToStore", parameters); | ||
| _results = _psHelper.ExecutePowerShell("Add-KeyfactorCertificate", parameters); | ||
| _logger.LogTrace("Returned from executing PS function (Add-KFCertificateToStore)"); | ||
|
|
Comment on lines
+269
to
+285
| _results = _psHelper.ExecutePowerShell("Add-KFCertificateToStore", parameters); | ||
| _logger.LogTrace("Returned from executing PS function (Add-KFCertificateToStore)"); | ||
| _results = _psHelper.ExecutePowerShell("Add-KeyfactorCertificate", parameters); | ||
| _logger.LogTrace("Returned from executing PS function (Add-KeyfactorCertificate)"); |
Comment on lines
149
to
164
| _logger.LogTrace("Returned from executing PS function (Add-KFCertificateToStore)"); | ||
| _results = _psHelper.ExecutePowerShell("Add-KeyfactorCertificate", parameters); | ||
| _logger.LogTrace("Returned from executing PS function (Add-KeyfactorCertificate)"); | ||
|
|
Comment on lines
+4
to
+8
| [string]$StorePath, | ||
|
|
||
| [parameter(ParameterSetName = $false)] | ||
| [switch]$IsAlias | ||
| ) |
Comment on lines
+31
to
+33
| Set-Location -Path "Cert:\LocalMachine\$StoreName" | ||
|
|
||
| $importResult = Import-Certificate -FilePath $cerFilename |
Comment on lines
+28
to
+36
| Write-Information "[VERBOSE] Instance: $instance" | ||
| Write-Information "[VERBOSE] Full Instance: $fullInstance" | ||
| Write-Information "[VERBOSE] Registry Location: $regLocation" | ||
| Write-Information "[VERBOSE] Current Thumbprint: $currentThumbprint" | ||
|
|
||
| $currentThumbprint = Get-ItemPropertyValue -Path $regLocation -Name "Certificate" -ErrorAction SilentlyContinue | ||
|
|
||
| if ($RenewalThumbprint -and $RenewalThumbprint -contains $currentThumbprint) { | ||
| Write-Information "Renewal thumbprint matches for instance: $fullInstance" |
Comment on lines
+35
to
+46
| if ($RenewalThumbprint -and $RenewalThumbprint -contains $currentThumbprint) { | ||
| Write-Information "Renewal thumbprint matches for instance: $fullInstance" | ||
| $result = Set-KeyfactorSQLCertificateBinding -InstanceName $instance -NewThumbprint $NewThumbprint -RestartService:$RestartService | ||
| } elseif (-not $RenewalThumbprint) { | ||
| Write-Information "No renewal thumbprint provided. Binding certificate to instance: $fullInstance" | ||
| $result = Set-KeyfactorSQLCertificateBinding -InstanceName $instance -NewThumbprint $NewThumbprint -RestartService:$RestartService | ||
| } | ||
|
|
||
| if (-not $result) { | ||
| Write-Error "Failed to bind certificate for instance: $instance" | ||
| $bindingSuccess = $false | ||
| } |
Comment on lines
+11
to
+15
| return $serviceUser | ||
| } else { | ||
| Write-Error "SQL Server instance '$SQLInstanceName' not found or no service user associated." | ||
| return $null | ||
| } |
Comment on lines
+41
to
+44
| var (username, password) = GetCredentials(); | ||
| if (string.IsNullOrEmpty(username) || string.IsNullOrEmpty(password)) | ||
| yield break; | ||
|
|
Comment on lines
+299
to
301
| _results = _psHelper.ExecutePowerShell("Add-KeyfactorCertificate", parameters); | ||
| _logger.LogTrace("Returned from executing PS function (Add-KFCertificateToStore)"); | ||
|
|
Comment on lines
164
to
168
| case "Skipped": | ||
| psResult = OrchestratorJobStatusJobResult.Failure; | ||
| failureMessage = ($"PowerShell function New-KFIISSiteBinding failed on step: {step} - message:\n {errorMessage}"); | ||
| failureMessage = ($"PowerShell function New-KeyfactorIISSiteBinding failed on step: {step} - message:\n {errorMessage}"); | ||
| _logger.LogDebug(failureMessage); | ||
| break; |
Comment on lines
+4
to
6
| Write-Host "Creating store type: WinCert" | ||
| kfutil store-types create WinCert | ||
|
|
Comment on lines
+1
to
+8
| #!/bin/bash | ||
| # Store Type creation script using kfutil | ||
| # Generated by Doctool | ||
|
|
||
| # Creates all 4 store types using kfutil. | ||
| # kfutil reads definitions from the Keyfactor integration catalog. | ||
| # | ||
| # Auth environment variables (first matching method is used): | ||
| # OAuth access token: KEYFACTOR_AUTH_ACCESS_TOKEN | ||
| # OAuth client creds: KEYFACTOR_AUTH_CLIENT_ID + KEYFACTOR_AUTH_CLIENT_SECRET | ||
| # + KEYFACTOR_AUTH_TOKEN_URL | ||
| # Basic auth (AD): KEYFACTOR_HOSTNAME + KEYFACTOR_USERNAME + KEYFACTOR_PASSWORD | ||
| # + KEYFACTOR_DOMAIN | ||
| # | ||
| # Auto-generated by doctool generate-store-type-scripts — do not edit by hand. | ||
| set -e | ||
|
|
||
| if ! command -v kfutil &> /dev/null; then | ||
| echo "kfutil could not be found. Please install kfutil" | ||
| echo "See https://github.com/Keyfactor/kfutil#quickstart" | ||
| exit 1 | ||
| fi | ||
| echo "Creating store type: WinCert" | ||
| kfutil store-types create WinCert |
Comment on lines
+8
to
+14
| $serviceUser = (Get-CimInstance -ClassName Win32_Service -Filter "Name='$SQLServiceName'").StartName | ||
|
|
||
| if ($serviceUser) { | ||
| return $serviceUser | ||
| } else { | ||
| Write-Error "SQL Server instance '$SQLInstanceName' not found or no service user associated." | ||
| return $null |
Comment on lines
+29
to
+35
| Write-Information "[VERBOSE] Full Instance: $fullInstance" | ||
| Write-Information "[VERBOSE] Registry Location: $regLocation" | ||
| Write-Information "[VERBOSE] Current Thumbprint: $currentThumbprint" | ||
|
|
||
| $currentThumbprint = Get-ItemPropertyValue -Path $regLocation -Name "Certificate" -ErrorAction SilentlyContinue | ||
|
|
||
| if ($RenewalThumbprint -and $RenewalThumbprint -contains $currentThumbprint) { |
Comment on lines
+24
to
+26
| Write-Information "Entering PowerShell Script: New-KFIISSiteBinding" -InformationAction SilentlyContinue | ||
| Write-Information "[VERBOSE] Parameters: $(($PSBoundParameters.GetEnumerator() | ForEach-Object { "$($_.Key): '$($_.Value)'" }) -join ', ')" | ||
|
|
Comment on lines
+113
to
+117
| catch { | ||
| $errorMessage = "Unexpected error in New-KFIISSiteBinding: $($_.Exception.Message)" | ||
| Write-Error $errorMessage | ||
| return New-KeyfactorResult -Status Error -Code 999 -Step UnexpectedError -ErrorMessage $errorMessage | ||
| } |
Comment on lines
+12
to
+16
| if ($build -ge 20348) { | ||
| # Windows Server 2022+ (IIS 10.0.20348+) | ||
| Write-Information "[VERBOSE] Detected Windows Server 2022 or later (Build: $build)" | ||
| return @(1, 4, 8, 16, 32, 64) # Include unknowns for testing | ||
| } |
Comment on lines
+5
to
+8
|
|
||
| [parameter(ParameterSetName = $false)] | ||
| [switch]$IsAlias | ||
| ) |
Comment on lines
+31
to
+33
| Set-Location -Path "Cert:\LocalMachine\$StoreName" | ||
|
|
||
| $importResult = Import-Certificate -FilePath $cerFilename |
Comment on lines
+2
to
+6
| { "Machine": "192.168.230.139", "StoreType": "WinIIS", "JEAEndpointName": "" }, | ||
| { "Machine": "192.168.230.139", "StoreType": "WinIIS", "JEAEndpointName": "keyfactor.wincert" }, | ||
| { "Machine": "{SQL-Server-IP-or-Hostname}", "StoreType": "WinSQL", "JEAEndpointName": "" }, | ||
| { "Machine": "{SQL-JEA-Server-IP-or-Hostname}", "StoreType": "WinSQL", "JEAEndpointName": "keyfactor.wincert" }, | ||
| { "Machine": "192.168.230.139", "StoreType": "WinCert", "JEAEndpointName": "" }, |
Comment on lines
+1
to
+8
| #!/bin/bash | ||
| # Store Type creation script using kfutil | ||
| # Generated by Doctool | ||
|
|
||
| # Creates all 4 store types using kfutil. | ||
| # kfutil reads definitions from the Keyfactor integration catalog. | ||
| # | ||
| # Auth environment variables (first matching method is used): | ||
| # OAuth access token: KEYFACTOR_AUTH_ACCESS_TOKEN | ||
| # OAuth client creds: KEYFACTOR_AUTH_CLIENT_ID + KEYFACTOR_AUTH_CLIENT_SECRET | ||
| # + KEYFACTOR_AUTH_TOKEN_URL | ||
| # Basic auth (AD): KEYFACTOR_HOSTNAME + KEYFACTOR_USERNAME + KEYFACTOR_PASSWORD | ||
| # + KEYFACTOR_DOMAIN | ||
| # | ||
| # Auto-generated by doctool generate-store-type-scripts — do not edit by hand. | ||
| set -e | ||
|
|
||
| if ! command -v kfutil &> /dev/null; then | ||
| echo "kfutil could not be found. Please install kfutil" | ||
| echo "See https://github.com/Keyfactor/kfutil#quickstart" | ||
| exit 1 | ||
| fi | ||
| echo "Creating store type: WinCert" | ||
| kfutil store-types create WinCert |
Comment on lines
+1
to
+8
| # Store Type creation script using kfutil | ||
| # Generated by Doctool | ||
|
|
||
| # Uncomment if kfutil is not in your PATH | ||
| # Set-Alias -Name kfutil -Value 'C:\Program Files\Keyfactor\kfutil\kfutil.exe' | ||
| Write-Host "Creating store type: WinCert" | ||
| kfutil store-types create WinCert | ||
|
|
||
| if ($null -eq (Get-Command "kfutil" -ErrorAction SilentlyContinue)) { | ||
| Write-Host "kfutil could not be found. Please install kfutil" | ||
| Write-Host "See https://github.com/Keyfactor/kfutil#quickstart" | ||
| exit 1 | ||
| } | ||
| Write-Host "Creating store type: IISU" | ||
| kfutil store-types create IISU |
Comment on lines
+4
to
+7
| [string]$StorePath, | ||
|
|
||
| [parameter(ParameterSetName = $false)] | ||
| [switch]$IsAlias |
Comment on lines
+28
to
+46
| Write-Information "[VERBOSE] Instance: $instance" | ||
| Write-Information "[VERBOSE] Full Instance: $fullInstance" | ||
| Write-Information "[VERBOSE] Registry Location: $regLocation" | ||
| Write-Information "[VERBOSE] Current Thumbprint: $currentThumbprint" | ||
|
|
||
| $currentThumbprint = Get-ItemPropertyValue -Path $regLocation -Name "Certificate" -ErrorAction SilentlyContinue | ||
|
|
||
| if ($RenewalThumbprint -and $RenewalThumbprint -contains $currentThumbprint) { | ||
| Write-Information "Renewal thumbprint matches for instance: $fullInstance" | ||
| $result = Set-KeyfactorSQLCertificateBinding -InstanceName $instance -NewThumbprint $NewThumbprint -RestartService:$RestartService | ||
| } elseif (-not $RenewalThumbprint) { | ||
| Write-Information "No renewal thumbprint provided. Binding certificate to instance: $fullInstance" | ||
| $result = Set-KeyfactorSQLCertificateBinding -InstanceName $instance -NewThumbprint $NewThumbprint -RestartService:$RestartService | ||
| } | ||
|
|
||
| if (-not $result) { | ||
| Write-Error "Failed to bind certificate for instance: $instance" | ||
| $bindingSuccess = $false | ||
| } |
Comment on lines
+29
to
+33
| # Step 4: Import the certificate into the specified store | ||
| Write-Verbose "Importing the certificate to the store: Cert:\LocalMachine\$StoreName" | ||
| Set-Location -Path "Cert:\LocalMachine\$StoreName" | ||
|
|
||
| $importResult = Import-Certificate -FilePath $cerFilename |
Comment on lines
+1
to
+8
| # Store Type creation script using kfutil | ||
| # Generated by Doctool | ||
|
|
||
| # Uncomment if kfutil is not in your PATH | ||
| # Set-Alias -Name kfutil -Value 'C:\Program Files\Keyfactor\kfutil\kfutil.exe' | ||
| Write-Host "Creating store type: WinCert" | ||
| kfutil store-types create WinCert | ||
|
|
||
| if ($null -eq (Get-Command "kfutil" -ErrorAction SilentlyContinue)) { | ||
| Write-Host "kfutil could not be found. Please install kfutil" | ||
| Write-Host "See https://github.com/Keyfactor/kfutil#quickstart" | ||
| exit 1 | ||
| } | ||
| Write-Host "Creating store type: IISU" | ||
| kfutil store-types create IISU |
Comment on lines
+1
to
+8
| #!/bin/bash | ||
| # Store Type creation script using kfutil | ||
| # Generated by Doctool | ||
|
|
||
| # Creates all 4 store types using kfutil. | ||
| # kfutil reads definitions from the Keyfactor integration catalog. | ||
| # | ||
| # Auth environment variables (first matching method is used): | ||
| # OAuth access token: KEYFACTOR_AUTH_ACCESS_TOKEN | ||
| # OAuth client creds: KEYFACTOR_AUTH_CLIENT_ID + KEYFACTOR_AUTH_CLIENT_SECRET | ||
| # + KEYFACTOR_AUTH_TOKEN_URL | ||
| # Basic auth (AD): KEYFACTOR_HOSTNAME + KEYFACTOR_USERNAME + KEYFACTOR_PASSWORD | ||
| # + KEYFACTOR_DOMAIN | ||
| # | ||
| # Auto-generated by doctool generate-store-type-scripts — do not edit by hand. | ||
| set -e | ||
|
|
||
| if ! command -v kfutil &> /dev/null; then | ||
| echo "kfutil could not be found. Please install kfutil" | ||
| echo "See https://github.com/Keyfactor/kfutil#quickstart" | ||
| exit 1 | ||
| fi | ||
| echo "Creating store type: WinCert" | ||
| kfutil store-types create WinCert |
Comment on lines
+2
to
+7
| { "Machine": "192.168.230.139", "StoreType": "WinIIS", "JEAEndpointName": "" }, | ||
| { "Machine": "192.168.230.139", "StoreType": "WinIIS", "JEAEndpointName": "keyfactor.wincert" }, | ||
| { "Machine": "{SQL-Server-IP-or-Hostname}", "StoreType": "WinSQL", "JEAEndpointName": "" }, | ||
| { "Machine": "{SQL-JEA-Server-IP-or-Hostname}", "StoreType": "WinSQL", "JEAEndpointName": "keyfactor.wincert" }, | ||
| { "Machine": "192.168.230.139", "StoreType": "WinCert", "JEAEndpointName": "" }, | ||
| { "Machine": "192.168.230.139", "StoreType": "WinCert", "JEAEndpointName": "keyfactor.wincert" } |
…s when running IIS jobs.
Fixed tempfile left behind when using ssh
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 83 out of 163 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (5)
IISU/PowerShell/Keyfactor.WinCert.SQL/Public/New-KeyfactorSQLBinding.ps1:41
$currentThumbprintis logged before it’s assigned, and the renewal check uses-containsagainst a string. This makes the renewal path always evaluate false and can incorrectly mark binding as failed whenRenewalThumbprintis provided.
IISU/PowerShell/Keyfactor.WinCert.Common/Public/Remove-KeyfactorCertificate.ps1:8[parameter(ParameterSetName = $false)]is not a valid attribute value (ParameterSetName must be a string). This will cause a parse error when importing the module.
WindowsCertStore.IntegrationTests/servers.json:7servers.jsonnow contains a hard-coded private IP (192.168.230.139). This is environment-specific and can leak internal network details; it also makes the sample file less reusable for other environments. Prefer placeholders or documentation-reserved example IPs.
scripts/store_types/bash/kfutil_create_store_types.sh:17- The script no longer checks whether
kfutilis installed or whether the user is logged in; it will now fail with a less actionable error when prerequisites aren’t met. Also,#!/bin/bashis less portable than#!/usr/bin/env bash.
scripts/store_types/powershell/kfutil_create_store_types.ps1:14 - The script no longer checks whether
kfutilis installed or whether the user is logged in. Without these guards, failures are harder to diagnose in automation.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 83 out of 163 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (11)
IISU/PowerShell/Keyfactor.WinCert.SQL/Public/New-KeyfactorSQLBinding.ps1:32
$currentThumbprintis logged before it is assigned, and the renewal-thumbprint comparison uses-containsagainst a string (which won’t behave as intended). Also, when a renewal thumbprint is provided but doesn’t match,$resultis never set, causing the bind to be treated as a failure.
IISU/PowerShell/Keyfactor.WinCert.Common/Public/Import-KeyfactorSignedCertificate.ps1:34Import-Certificaterequires a-CertStoreLocation(and changing location viaSet-Locationis a side effect that can leak outside the function). This currently risks importing to the wrong store or failing outright.
IISU/PowerShell/Keyfactor.WinCert.Common/Public/Remove-KeyfactorCertificate.ps1:7[Parameter(ParameterSetName = $false)]is not a valid use ofParameterSetNameand can cause confusing parameter binding behavior. This switch should just be a normal optional parameter.
IISU/PowerShell/Keyfactor.WinCert.Common/Public/Remove-KeyfactorCertificate.ps1:22$store.Remove($cert)expects a singleX509Certificate2, but$certhere is the result of a pipeline filter and can be a collection. That will either throw or remove unpredictably when multiple matches exist (e.g., alias collisions).
IISU/PowerShell/Keyfactor.WinCert.IIS/Public/New-KeyfactorIISSiteBinding.ps1:24- The log message still references the old function name
New-KFIISSiteBinding, which makes troubleshooting harder now that the function isNew-KeyfactorIISSiteBinding.
IISU/PowerShell/Keyfactor.WinCert.IIS/Public/New-KeyfactorIISSiteBinding.ps1:116 - The thrown/captured error message still references
New-KFIISSiteBinding, which no longer matches the function name and will mislead operators when diagnosing failures.
IISU/PowerShell/Keyfactor.WinCert.IIS/Private/Get-ValidSslFlagsForSystem.ps1:16 - For Windows Server 2022+ this returns flag bits marked as "Include unknowns for testing" (16/32/64). That causes
Test-ValidSslFlagsto accept unsupported flags in production. The valid-bit list should only include IIS-documentedsslFlagsbits for the target OS/IIS version (or be discovered dynamically).
WindowsCertStore.IntegrationTests/servers.json:6 - This integration-test config currently hardcodes a specific private IP (192.168.230.139). That makes the repo environment-specific and risks accidentally leaking internal infrastructure details. Prefer placeholders/example values only.
scripts/store_types/bash/kfutil_create_store_types.sh:5 - The script now assumes
kfutilis installed and available, so failures will be a generic "command not found". Adding an explicit availability check improves usability and makes CI failures clearer. Also, using/usr/bin/env bashis more portable than hardcoding/bin/bash.
scripts/store_types/powershell/kfutil_create_store_types.ps1:5 - The script assumes
kfutilis installed/available; when it isn’t, users get a less actionable error. Add an explicitGet-Commandcheck to fail fast with guidance.
WindowsCertStore.UnitTests/SANsUnitTests.cs:86 - This test can be simplified: both
nulland""are expected to yield an empty result, and the commented-out assert should be removed to avoid confusion.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 83 out of 163 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (5)
IISU/PowerShell/Keyfactor.WinCert.SQL/Public/New-KeyfactorSQLBinding.ps1:35
- $currentThumbprint is logged before it’s assigned, and the renewal check uses
-contains(collection membership) which won’t work for comparing thumbprint strings. When$RenewalThumbprintis provided but doesn’t match,$resultis never set, so the function will report failure even though it should simply skip binding for that instance.
IISU/PowerShell/Keyfactor.WinCert.Common/Public/Remove-KeyfactorCertificate.ps1:8 [parameter(ParameterSetName = $false)]is not a valid use ofParameterSetNameand can create an unintended parameter set namedFalse. If there’s only one parameter set here, the attribute line should be removed (or replaced with a normal[Parameter()]).
IISU/PowerShell/Keyfactor.WinCert.Common/Public/Remove-KeyfactorCertificate.ps1:29$store.Remove($cert)will fail when$certis a collection (e.g., multiple matches fromWhere-Object). Iterate and remove each certificate explicitly (and keep behavior consistent for single/multiple matches).
IISU/PowerShell/Keyfactor.WinCert.IIS/Private/Get-ValidSslFlagsForSystem.ps1:16- The Server 2022+ branch advertises unsupported/unknown flag bits (
16, 32, 64) as valid (per the comment), while the validator also treats bit0x2(CentralCertStore) as a known flag (seeTest-ValidSslFlags), but0x2is not included in the 2022+ valid list. This makes validation inconsistent and can allow invalid flags through (or incorrectly reject CentralCertStore).
WindowsCertStore.UnitTests/SANsUnitTests.cs:96 - This theory contains a commented-out assert and redundant branching (
legacySan == nullandlegacySan == string.Emptyboth expect the same result). This makes the test harder to read and maintain without changing behavior.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 83 out of 163 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (9)
IISU/PowerShell/Keyfactor.WinCert.SQL/Public/New-KeyfactorSQLBinding.ps1:33
$currentThumbprintis logged before it is assigned, so the verbose output will always be empty/incorrect. Assign the value first, then log it.
IISU/PowerShell/Keyfactor.WinCert.SQL/Public/New-KeyfactorSQLBinding.ps1:41- Renewal thumbprint matching uses
-containsagainst a string and has no branch when a non-matching renewal thumbprint is provided. This can cause bindings to be skipped silently and$resultto be uninitialized (treated as failure). Split the renewal thumbprint list and explicitly handle the non-match case.
IISU/PowerShell/Keyfactor.WinCert.IIS/Private/Get-ValidSslFlagsForSystem.ps1:16 - This function claims to return valid SslFlags bits, but the WS2022+ branch explicitly includes "unknowns for testing" and also omits bit 0x2 (CentralCertStore) even though
Test-ValidSslFlagschecks for it. This will incorrectly reject legitimate flag combinations and/or accept unsupported bits.
IISU/PowerShell/Keyfactor.WinCert.IIS/Public/New-KeyfactorIISSiteBinding.ps1:24 - The informational log line still references the old function name
New-KFIISSiteBinding, which makes troubleshooting confusing.
IISU/PowerShell/Keyfactor.WinCert.IIS/Public/New-KeyfactorIISSiteBinding.ps1:117 - The catch block error message still references the old function name
New-KFIISSiteBinding, which can mislead during incident triage.
IISU/PowerShell/Keyfactor.WinCert.Common/Public/Remove-KeyfactorCertificate.ps1:8 [Parameter(ParameterSetName = $false)]is not a meaningful parameter set name and will be coerced to the literal string "False". If you don't intend to use parameter sets here, remove this attribute to avoid confusing parameter metadata.
IISU/PowerShell/Keyfactor.WinCert.Common/Public/Remove-KeyfactorCertificate.ps1:35$certcan be an array (multiple matches), butX509Store.Remove()expects a singleX509Certificate2. Also, throwing when the certificate isn't found forces the caller into exception handling for a common "not found" outcome. Iterate over matches and return$falsewhen nothing is found.
IISU/PowerShell/Keyfactor.WinCert.Common/Public/Remove-KeyfactorCertificate.ps1:42- The catch block rethrows (
throw $_), which will fail the remote PowerShell invocation with an exception rather than returning a simple success/failure signal. Since this function already tracks$isSuccessful, prefer returning$falsefrom the catch block.
IISU/PowerShell/Keyfactor.WinCert.Common/Public/Import-KeyfactorSignedCertificate.ps1:33 Set-Locationchanges the caller's current location and isn't restored, which can have surprising side effects in long-lived runspaces.Import-Certificatesupports importing directly to a certificate store location, so you can avoid changing location entirely.
Comment on lines
38
to
+39
| var json = File.ReadAllText("servers.json"); | ||
| var machines = JsonConvert.DeserializeObject<List<Dictionary<string, string>>>(json); | ||
| var connections = JsonConvert.DeserializeObject<List<ClientConnection>>(json) ?? new(); |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 83 out of 163 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (8)
IISU/PowerShell/Keyfactor.WinCert.SQL/Public/New-KeyfactorSQLBinding.ps1:35
$currentThumbprintis logged before it’s assigned, and the renewal-thumbprint check uses-containson a string (which will never match). This can cause renewal matching to fail even when the thumbprints are identical.
IISU/PowerShell/Keyfactor.WinCert.Common/Public/Remove-KeyfactorCertificate.ps1:7ParameterSetName = $falseis not a valid value for the[Parameter()]attribute and can cause parameter binding/parse errors. If this switch isn’t part of a parameter set, omitParameterSetNameentirely.
IISU/PowerShell/Keyfactor.WinCert.IIS/Private/Get-ValidSslFlagsForSystem.ps1:16- This returns flag values explicitly marked as “unknowns for testing”. That makes
Test-ValidSslFlagsaccept unsupportedSslFlagsbits in production, defeating the validation.
IISU/PowerShell/Keyfactor.WinCert.Common/Public/Import-KeyfactorSignedCertificate.ps1:33 - Using
Set-Locationto change the session’s current provider path is a side effect that can break subsequent operations in the same runspace.Import-Certificatesupports importing directly to a target store via-CertStoreLocation.
IISU/PowerShell/Keyfactor.WinCert.SQL/Public/Get-KeyfactorSQLInventory.ps1:51 $cert.PrivateKey.CspKeyContainerInfo.ProviderNamecan throw or be$nullfor CNG/ECC keys. The Common module already hasGet-CertificateCSPto safely resolve provider/KSP names; using it here avoids inventory failures on non-CSP certificates.
WindowsCertStore.IntegrationTests/servers.json:3- This integration-test config file now contains a concrete private IP address (
192.168.230.139). Committing environment-specific addresses makes the tests/docs harder to reuse and can leak internal network details; prefer placeholders like the existing{SQL-...}entries.
integration-manifest.json:352 - This PR is titled as adding JEA support, but this change flips WinSql
SupportedOperations.Enrollmentfromfalsetotrue, which is a functional capability change for the store type. Please confirm this is intentional and update the PR description/release notes accordingly (or revert if accidental).
scripts/store_types/bash/kfutil_create_store_types.sh:1 - Using a hardcoded
#!/bin/bashshebang reduces portability (e.g., systems where bash is not at/bin/bash).#!/usr/bin/env bashis more robust and matches the previous version.
Comment on lines
+44
to
+50
| Add-Type -Path "$env:windir\System32\inetsrv\Microsoft.Web.Administration.dll" | ||
| $iis = New-Object Microsoft.Web.Administration.ServerManager | ||
| $site = $iis.Sites[$SiteName] | ||
|
|
||
| $httpsBindings = $site.Bindings | Where-Object { | ||
| $_.BindingInformation -eq $BindingInfo -and $_.Protocol -eq "https" | ||
| } |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 83 out of 163 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (5)
IISU/PowerShell/Keyfactor.WinCert.SQL/Public/New-KeyfactorSQLBinding.ps1:41
$currentThumbprintis logged before it’s assigned, and-containsis the wrong operator for checking whether a (comma-separated) renewal thumbprint string matches the currently bound thumbprint. This can cause false mismatches (or always-fail) renewals and also makes$resultpotentially uninitialized when a renewal thumbprint is provided but doesn’t match.
IISU/PowerShell/Keyfactor.WinCert.SQL/Public/Get-KeyfactorSQLInventory.ps1:51- Provider name extraction uses
$cert.PrivateKey.CspKeyContainerInfo.ProviderName, which breaks for CNG-backed keys (e.g., RSACng/ECDsa) becauseCspKeyContainerInfoisn’t available. The Common module already providesGet-CertificateCSPwhich handles both legacy CSP and CNG keys.
IISU/PowerShell/Keyfactor.WinCert.Common/Public/Remove-KeyfactorCertificate.ps1:6 [parameter(ParameterSetName = $false)]is invalid/misleading here (ParameterSetName expects a string). As written it coerces to the literal set name "False", which can cause confusing parameter binding behavior.
WindowsCertStore.IntegrationTests/servers.json:7- This committed
servers.jsonincludes a specific private IP (192.168.230.139). This makes the repo’s default integration-test config non-portable and risks leaking internal environment details. Prefer placeholders only (or document that users should copy to a local, untracked file).
IISU/ImplementedStoreTypes/WinAdfs/AdfsCertificateRotationManager.cs:559 - The
catch (Exception) { throw; }block is redundant and adds noise without changing behavior. Removing it keeps the control flow clearer.
catch (Exception)
{
throw;
}
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.