Conversation
3.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 the Entry Parameter "SAN" from the integration-manifest.json, but will still support previous versions of Command in the event the SAN Entry Parameter is passed. The next major version (4.0) will remove all support for the SAN Entry Parameter. * Added WinADFS Store Type for rotating certificates in ADFS environments. Please note, only the service-communications certificate is rotated throughout your farm. * Internal only: Added Integration Tests to aid in future development and testing. * Improved messaging in the event an Entry Parameter is missing (or does not meet the casing requirements) * Fixed the SNI/SSL flag being returned during inventory, now returns extended SSL flags * Fixed the SNI/SSL flag when binding the certificate to allow for extended SSL flags * Added SSL Flag validation to make sure the bit flag is correct. These are the valid bit flags for the version of Windows: ### Windows Server 2012 R2 / Windows 8.1 and earlier (IIS 8.5): * 0 No SNI * 1 Use SNI * 2 Use Centralized SSL certificate store. ### Windows Server 2016 (IIS 10.0): * 0 No SNI * 1 Use SNI * 4 Disable HTTP/2. ### Windows Server 2019 (IIS 10.0.17763) * 0 No SNI * 1 Use SNI * 4 Disable HTTP/2. * 8 Disable OCSP Stapling. ### Windows Server 2022+ (IIS 10.0.20348+) * 0 No SNI * 1 Use SNI * 4 Disable HTTP/2. * 8 Disable OCSP Stapling. * 16 Disable QUIC. * 32 Disable TLS 1.3 over TCP. * 64 Disable Legacy TLS. --------- Co-authored-by: Bob Pokorny <bpokorny@keyfactor.com> Co-authored-by: Keyfactor <keyfactor@keyfactor.github.io>
There was a problem hiding this comment.
Pull request overview
Automated merge of the release-3.0 branch into main, delivering the 3.0.0 feature set for the Windows Certificate Orchestrator extension (notably WinADFS support, SAN handling changes, IIS SSL flags improvements, plus expanded docs/tests).
Changes:
- Adds a new WinADFS store type (manifest + PowerShell + C# job implementations + docs).
- Updates IIS SSL flags handling/validation and adjusts related documentation.
- Introduces new Unit/Integration test projects and supporting utilities.
Reviewed changes
Copilot reviewed 40 out of 79 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| integration-manifest.json | Removes legacy SAN entry parameter and adds WinADFS store type definition. |
| docsource/winadfs.md | Adds WinADFS documentation page. |
| docsource/images/WinCert-custom-field-spnwithport-validation-options-dialog.png | Adds new documentation screenshot asset. |
| docsource/iisu.md | Expands IISU docs with SSL flag details; introduces an “Overview” placeholder section. |
| docsource/content.md | Updates integration overview to include WinADFS and job-type note. |
| WindowsCertStore.sln | Adds IntegrationTests/UnitTests projects; removes docsource/images SolutionItems sections. |
| WindowsCertStore.UnitTests/WindowsCertStore.UnitTests.csproj | Adds new .NET test project configuration and references. |
| WindowsCertStore.UnitTests/SANsUnitTests.cs | Adds unit tests for SANBuilder and SAN resolution logic. |
| WindowsCertStore.UnitTests/PSHelperUnitTests.cs | Adds unit test for loading PowerShell scripts. |
| WindowsCertStore.UnitTests/CertificateUnitTests.cs | Adds unit test for writing certificates to temp PFX files. |
| WindowsCertStore.UnitTests/AdfsUnitTests.cs | Adds an ADFS “unit test” that currently depends on live infrastructure. |
| WindowsCertStore.IntegrationTests/servers.json | Adds integration test server list placeholder. |
| WindowsCertStore.IntegrationTests/appsettings.json | Adds integration test Key Vault configuration placeholder. |
| WindowsCertStore.IntegrationTests/WindowsCertStore.IntegrationTests.csproj | Adds integration test project and external package dependencies (Azure Key Vault, etc.). |
| WindowsCertStore.IntegrationTests/WinSQLIntegrationTests.cs | Adds end-to-end WinSQL add/inventory/remove integration tests. |
| WindowsCertStore.IntegrationTests/WinIISIntegrationTests.cs | Adds end-to-end WinIIS add/inventory/remove integration tests. |
| WindowsCertStore.IntegrationTests/VaultHelper.cs | Adds Key Vault helper for integration tests. |
| WindowsCertStore.IntegrationTests/Factories/ConnectionFactory.cs | Adds factory that loads servers + retrieves credentials from Key Vault. |
| WindowsCertStore.IntegrationTests/Factories/ConfigurationFactory.cs | Adds job configuration builders for integration tests. |
| WindowsCertStore.IntegrationTests/Factories/CertificateFactory.cs | Adds self-signed certificate generator for integration tests. |
| WindowsCertStore.IntegrationTests/ClientConnection.cs | Adds integration test connection model. |
| README.md | Updates top-level docs for WinADFS and screenshots; introduces an IISU “TODO” placeholder. |
| IISU/manifest.json | Registers WinADFS Inventory/Management capabilities. |
| IISU/WindowsCertStore.csproj | Updates PowerShell SDK version; ensures WinADFSScripts.ps1 is copied to output. |
| IISU/SANBuilder.cs | Adds SANBuilder helper for building SAN strings from structured SANs. |
| IISU/PowerShellScripts/WinCertScripts.ps1 | Updates WinCert/IIS/SQL PowerShell functions; adds SSL flag validation and thumbprint filtering. |
| IISU/PowerShellScripts/WinADFSScripts.ps1 | Adds ADFS PowerShell functions for inventory and rotation tasks. |
| IISU/PSHelper.cs | Refactors script loading to support multiple .ps1 files; adds ADFS-local-only constraint and helpers. |
| IISU/JobConfigurationParser.cs | Improves null-safety when parsing job configuration for logging. |
| IISU/ImplementedStoreTypes/WinIIS/Management.cs | Minor refactor to centralize IISBindingInfo creation; adds additional logging/cleanup guard. |
| IISU/ImplementedStoreTypes/WinIIS/IISBindingInfo.cs | Adds a SslFlags enum and improves missing-entry-parameter error messaging. |
| IISU/ImplementedStoreTypes/WinAdfs/Management.cs | Adds WinADFS management job implementation (certificate rotation). |
| IISU/ImplementedStoreTypes/WinAdfs/CertificateRotationResult.cs | Adds result model for ADFS rotation operations. |
| IISU/ImplementedStoreTypes/WinAdfs/AdfsNodeInfo.cs | Adds node status model for ADFS farm operations. |
| IISU/ImplementedStoreTypes/WinAdfs/AdfsInventory.cs | Adds farm inventory model for ADFS operations. |
| IISU/ImplementedStoreTypes/WinAdfs/AdfsCertificateRotationManager.cs | Adds ADFS farm certificate rotation orchestration logic. |
| IISU/ImplementedStoreTypes/WinAdfs/AdfsCertificateInfo.cs | Adds ADFS certificate information model. |
| IISU/ImplementedStoreTypes/WinADFS/Inventory.cs | Adds WinADFS inventory job implementation. |
| IISU/ClientPSCertStoreReEnrollment.cs | Makes class public; adds SAN resolution supporting both structured SANs and legacy SAN string. |
| IISU/Certificate.cs | Adds temp PFX write + cleanup helpers. |
| CHANGELOG.md | Documents 3.0.0 feature set and SSL flag rules. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| [Theory] | ||
| [MemberData(nameof(ConnectionFactory.GetConnection), MemberType = typeof(ConnectionFactory))] | ||
| public void WinIIS_Management_Add_Inventory_Remove_EndToEnd_Test(ClientConnection connection) | ||
| { | ||
| var (thumbprint, base64Pfx, pfxPassword) = CreateTestCertificate(); |
There was a problem hiding this comment.
These tests appear to require live infrastructure (reachable target machines and credentials) via ConnectionFactory.GetConnection. If CI runs dotnet test on the solution, they will fail by default. Please mark them as explicitly skippable/opt-in (trait/category + CI filter, skip unless env vars are present, etc.) and document how to run them locally.
| else { | ||
| # Windows Server 2012 R2 and earlier (IIS 8.5) | ||
| Write-Verbose "Detected Windows Server 2012 R2 or earlier (Build: $build)" | ||
| return @(1, 2) | ||
| } | ||
| } | ||
| function Test-ValidSslFlags { | ||
| param([int]$SslFlags) | ||
|
|
||
| $validBits = 1,2,4,8,32,64,128 | ||
| $invalidBits = $SslFlags -bxor ($SslFlags -band ($validBits | Measure-Object -Sum).Sum) | ||
|
|
||
| return ($invalidBits -eq 0) | ||
| [CmdletBinding()] | ||
| param( | ||
| [Parameter(Mandatory = $true)] | ||
| [int]$Flags, | ||
|
|
||
| [Parameter(Mandatory = $false)] | ||
| [switch]$ThrowOnError | ||
| ) | ||
|
|
||
| $build = [System.Environment]::OSVersion.Version.Build | ||
| $validBits = Get-ValidSslFlagsForSystem | ||
|
|
||
| # Calculate valid bitmask | ||
| $validMask = 0 | ||
| foreach ($bit in $validBits) { | ||
| $validMask = $validMask -bor $bit | ||
| } | ||
|
|
||
| # Check for unknown/unsupported bits | ||
| $unknownBits = $Flags -band (-bnot $validMask) | ||
| if ($unknownBits -ne 0) { | ||
| $errorMsg = "SslFlags value $Flags (0x$($Flags.ToString('X'))) contains unsupported bits " + | ||
| "for this Windows Server version (Build: $build): $unknownBits (0x$($unknownBits.ToString('X'))). " + | ||
| "Supported flags: $($validBits -join ', ')" | ||
|
|
||
| if ($ThrowOnError) { | ||
| throw $errorMsg | ||
| } | ||
| else { | ||
| return [PSCustomObject]@{ | ||
| IsValid = $false | ||
| ErrorCode = 400 | ||
| Message = $errorMsg | ||
| WindowsBuild = $build | ||
| ValidFlags = $validBits | ||
| InvalidBits = $unknownBits | ||
| } | ||
| } | ||
| } | ||
|
|
||
| # Check for known invalid combinations | ||
| $hasSni = ($Flags -band 1) -ne 0 | ||
| $hasCentralCert = ($Flags -band 2) -ne 0 | ||
|
|
||
| if ($hasCentralCert -and -not $hasSni) { | ||
| $errorMsg = "SslFlags value $Flags (0x$($Flags.ToString('X'))) is invalid: " + | ||
| "CentralCertStore (0x2) requires SNI (0x1) to be enabled." | ||
|
|
There was a problem hiding this comment.
Test-ValidSslFlags currently treats the Centralized Certificate Store bit (0x2) as valid (when combined with SNI), but the docs state CCS is not supported and should error. This mismatch can lead to jobs proceeding with an unsupported configuration. Consider rejecting any SslFlags value containing bit 0x2 (and updating Get-ValidSslFlagsForSystem accordingly) or updating the docs if CCS is intended to be supported.
| ## Overview | ||
|
|
||
| TODO Overview is a required section | ||
|
|
There was a problem hiding this comment.
The IISU doc page ends with a placeholder section:
`## Overview
TODO Overview is a required section`
This will leak into generated docs. Please replace the TODO with the actual overview content or remove the section if it’s no longer required.
| [Fact] | ||
| public void Test_AdfsInventory() | ||
| { | ||
| // Arrange | ||
| RemoteSettings settings = new RemoteSettings | ||
| { | ||
| ClientMachineName = "{IPAddress}}", | ||
| Protocol = "http", | ||
| Port = "5985", | ||
| IncludePortInSPN = true, | ||
| ServerUserName = @"{username}", | ||
| ServerPassword = "{password}" | ||
| }; | ||
|
|
||
| // Act | ||
| Keyfactor.Extensions.Orchestrator.WindowsCertStore.WinAdfs.Inventory adfs = new(); | ||
| adfs.QueryWinADFSCertificates(settings, "My"); | ||
|
|
There was a problem hiding this comment.
This test is not a unit test: it attempts to connect to a real remote ADFS server (placeholder values) and has no assertions. As written it will be flaky/fail in CI. Consider converting to a mocked unit test, moving it into the IntegrationTests project, or marking it skipped with a clear reason.
| string scriptsFolder = PSHelper.FindScriptsDirectory(AppDomain.CurrentDomain.BaseDirectory, "PowerShellScripts"); | ||
|
|
||
| // Act | ||
| string scripts = psHelper.LoadAllScripts(scriptsFolder); | ||
| // Assert | ||
| scripts.Contains("# All scripts loaded."); | ||
|
|
There was a problem hiding this comment.
The assertion is currently ineffective: scripts.Contains(...) does not fail the test when false. Also, FindScriptsDirectory(...) can return null, which will throw in LoadAllScripts. Please use Assert.Contains(...) and assert/arrange that the PowerShellScripts folder is available in the test output (or copy scripts into the test project’s output during build).
| ### IISU | ||
|
|
||
| <details><summary>Click to expand details</summary> | ||
|
|
||
|
|
||
| The IIS Bound Certificate Store Type, identified by its short name 'IISU,' is designed for the management of certificates bound to IIS (Internet Information Services) servers. This store type allows users to automate and streamline the process of adding, removing, and reenrolling certificates for IIS sites, making it significantly easier to manage web server certificates. | ||
|
|
||
| #### Key Features and Representation | ||
|
|
||
| The IISU store type represents the IIS servers and their certificate bindings. It specifically caters to managing SSL/TLS certificates tied to IIS websites, allowing bind operations such as specifying site names, IP addresses, ports, and enabling Server Name Indication (SNI). By default, it supports job types like Inventory, Add, Remove, and Reenrollment, thereby offering comprehensive management capabilities for IIS certificates. | ||
|
|
||
| #### Limitations and Areas of Confusion | ||
|
|
||
| - **Caveats:** It's important to ensure that the Windows Remote Management (WinRM) is properly configured on the target server. The orchestrator relies on WinRM to perform its tasks, such as manipulating the Windows Certificate Stores. Misconfiguration of WinRM may lead to connection and permission issues. | ||
| <br><br>When performing <b>Inventory</b>, all bound certificates <i>regardless</i> to their store location will be returned. | ||
| <br><br>When executing an Add or Renew Management job, the Store Location will be considered and place the certificate in that location. | ||
|
|
||
| - **Limitations:** Users should be aware that for this store type to function correctly, certain permissions are necessary. While some advanced users successfully use non-administrator accounts with specific permissions, it is officially supported only with Local Administrator permissions. Complexities with interactions between Group Policy, WinRM, User Account Control, and other environmental factors may impede operations if not properly configured. | ||
|
|
||
| - **Custom Alias and Private Keys:** The store type does not support custom aliases for individual entries and requires private keys because IIS certificates without private keys would be invalid. | ||
| TODO Overview is a required section |
There was a problem hiding this comment.
The IISU documentation section currently contains a placeholder (“TODO Overview is a required section”), which removes the actual store-type overview from the published README. Please restore real IISU overview content (or remove the placeholder) before release.
| The returned list will contain the actual certificate store name to be used when entering store location. | ||
|
|
||
| This extension implements four job types: Inventory, Management Add/Remove, and Reenrollment. | ||
| The ADFS extension performs both Inventory and Management Add jobs. The other extensions implements four job types: Inventory, Management Add/Remove, and Reenrollment. |
There was a problem hiding this comment.
Grammar: “The other extensions implements” should be “The other extensions implement”.
| The returned list will contain the actual certificate store name to be used when entering store location. | ||
|
|
||
| This extension implements four job types: Inventory, Management Add/Remove, and Reenrollment. | ||
| The ADFS extension performs both Inventory and Management Add jobs. The other extensions implements four job types: Inventory, Management Add/Remove, and Reenrollment. |
There was a problem hiding this comment.
Grammar: “The other extensions implements” should be “The other extensions implement”.
| string tempFilePath = Keyfactor.Extensions.Orchestrator.WindowsCertStore.Certificate.Utilities.WriteCertificateToTempPfx(base64Cert); | ||
|
|
||
| // Assert | ||
| Assert.False(string.IsNullOrEmpty(tempFilePath)); | ||
| Assert.True(System.IO.File.Exists(tempFilePath)); | ||
|
|
There was a problem hiding this comment.
This test writes a temp .pfx file but never cleans it up, which can pollute developer machines/CI runners over time. Please call Certificate.Utilities.CleanupTempCertificate(tempFilePath) in a finally block (and consider asserting the file is deleted).
| [Theory] | ||
| [MemberData(nameof(ConnectionFactory.GetConnection), MemberType = typeof(ConnectionFactory))] | ||
| public void WinSql_Management_Add_Inventory_Remove_EndToEnd_Test(ClientConnection connection) | ||
| { |
There was a problem hiding this comment.
These tests appear to require live infrastructure (reachable target machines + Azure Key Vault + credentials) via ConnectionFactory.GetConnection. If CI runs dotnet test on the solution, they will fail by default. Please mark them as explicitly skippable/opt-in (e.g., trait/category + CI filter, [Fact(Skip=...)] unless env vars are present, or use Xunit.SkippableFact) and document how to run them locally.
Merge release-3.0 to main - Automated PR