76022 iis orchestrator 3.0.0 Ready for release#178
76022 iis orchestrator 3.0.0 Ready for release#178rcpokorny wants to merge 65 commits intorelease-3.0from
Conversation
Completed adding Integration Tests
…r SNI retrieval. - Enhanced `New-KFIISSiteBinding` to robustly handle SSL flags, including checks for managed API range and updating extended flags via `appcmd.exe`. - Improved comments and verbose logging for better clarity and debugging.
…rrectly 76938 sni flag not reporting correctly
…N_Entry_Parameter 76023 eliminate but support san entry parameter
…ocal and remote sessions.
… additional "housekeeping" for ADFS.
…tor/iis-orchestrator into 60764_Adding_ADFS_Support
60764 adding adfs support
Updated SAN handling in integration-manifest.json and clarified support for previous versions. Added new features and improvements.
Updated changelog to reflect changes in version 3.0.0, including SAN handling and new WinADFS Store Type.
Added SSL Flag validation details and improved messaging.
Updated SSL Flag descriptions for clarity and accuracy.
There was a problem hiding this comment.
Pull request overview
This PR releases version 3.0.0 of the IIS orchestrator extension with significant changes to SAN handling, a new WinADFS store type for ADFS certificate rotation, integration tests, and various bug fixes related to SSL flags and error messaging.
Changes:
- SANs are now handled through ODKG Enrollment in Command rather than the SAN Entry Parameter (backward compatibility maintained in 3.0)
- Added WinADFS store type for automated ADFS service-communications certificate rotation across farm nodes
- Fixed SNI/SSL flag handling in IIS bindings with validation for correct bit flag combinations
Reviewed changes
Copilot reviewed 39 out of 52 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| integration-manifest.json | Removed SAN entry parameters from WinCert, IISU, and WinSql store types; added new WinAdfs store type configuration |
| docsource/winadfs.md | Added documentation for the new WinADFS store type |
| WindowsCertStore.sln | Added integration and unit test projects to solution |
| WindowsCertStore.UnitTests/* | Added unit test project and tests for SANs, PowerShell helpers, certificates, and ADFS |
| WindowsCertStore.IntegrationTests/* | Added integration test project with end-to-end tests for WinSQL and WinIIS |
| IISU/SANBuilder.cs | New class for building SAN strings from dictionary input |
| IISU/PowerShellScripts/WinCertScripts.ps1 | Updated version to 1.5.0 with SSL flag fixes and certificate retrieval improvements |
| IISU/PowerShellScripts/WinADFSScripts.ps1 | New PowerShell script with ADFS management functions |
| IISU/PSHelper.cs | Enhanced to support multiple script loading, ADFS module import, and improved error handling |
| IISU/ImplementedStoreTypes/WinIIS/* | Added SSL flag validation and improved error messaging |
| IISU/ImplementedStoreTypes/WinAdfs/* | New implementation for ADFS certificate rotation manager |
| IISU/ClientPSCertStoreReEnrollment.cs | Updated to support new SAN handling with backward compatibility |
| README.md | Updated documentation to include WinADFS store type |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
WindowsCertStore.IntegrationTests/Factories/ConfigurationFactory.cs
Outdated
Show resolved
Hide resolved
…of Windows Server and IIS.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 39 out of 52 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
WindowsCertStore.IntegrationTests/Factories/ConfigurationFactory.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 40 out of 53 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…th adding links to Microsoft documentation for clarification.
…factor/iis-orchestrator into 76022-IIS_Orchestrator-3.0.0
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 40 out of 53 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…factor/iis-orchestrator into 76022-IIS_Orchestrator-3.0.0
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 40 out of 79 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -344,6 +633,77 @@ public void Terminate() | |||
| return results; | |||
| } | |||
There was a problem hiding this comment.
The method InvokeFunctionOLD appears to be legacy code that should be removed. Old method implementations should be cleaned up before a major version release.
| @@ -278,12 +557,17 @@ public void Terminate() | |||
| { | |||
There was a problem hiding this comment.
There are two old initialization methods (InitializeLocalSessionOLD and InitializeLocalSessionOLD2) that should be removed before release. These legacy methods add unnecessary code complexity.
| function Grant-AdfsCertificatePermissionsOLD { | ||
| <# | ||
| .SYNOPSIS | ||
| Grant ADFS service account access to certificate private key | ||
| .PARAMETER CertificateThumbprint | ||
| Thumbprint of the certificate | ||
| .PARAMETER ServiceAccountName | ||
| Name of the ADFS service account | ||
| #> | ||
| param( | ||
| [Parameter(Mandatory=$true)] | ||
| [string]$CertificateThumbprint, | ||
|
|
||
| [Parameter(Mandatory=$true)] | ||
| [string]$ServiceAccountName | ||
| ) | ||
|
|
||
| try { | ||
| Write-Verbose "Granting permissions to service account on $env:COMPUTERNAME..." | ||
|
|
||
| # Get the certificate | ||
| $cert = Get-ChildItem -Path "Cert:\LocalMachine\My\$CertificateThumbprint" -ErrorAction Stop | ||
|
|
||
| if (-not $cert.HasPrivateKey) { | ||
| throw "Certificate does not have a private key" | ||
| } | ||
|
|
||
| # Get the private key | ||
| $rsaCert = [System.Security.Cryptography.X509Certificates.RSACertificateExtensions]::GetRSAPrivateKey($cert) | ||
| $fileName = $rsaCert.Key.UniqueName | ||
|
|
||
| # Private keys are stored here | ||
| $privateKeyPath = "$env:ProgramData\Microsoft\Crypto\RSA\MachineKeys\$fileName" | ||
|
|
||
| if (Test-Path $privateKeyPath) { | ||
| # Get current ACL | ||
| $acl = Get-Acl -Path $privateKeyPath | ||
|
|
||
| # Create access rule for service account | ||
| $accessRule = New-Object System.Security.AccessControl.FileSystemAccessRule( | ||
| $ServiceAccountName, | ||
| "Read", | ||
| "Allow" | ||
| ) | ||
|
|
||
| # Add the access rule | ||
| $acl.AddAccessRule($accessRule) | ||
|
|
||
| # Set the ACL | ||
| Set-Acl -Path $privateKeyPath -AclObject $acl | ||
|
|
||
| Write-Host "✓ Permissions granted to $ServiceAccountName" | ||
|
|
||
| return [PSCustomObject]@{ | ||
| Success = $true | ||
| ServiceAccount = $ServiceAccountName | ||
| PrivateKeyPath = $privateKeyPath | ||
| NodeName = $env:COMPUTERNAME | ||
| } | ||
| } | ||
| else { | ||
| throw "Private key file not found at $privateKeyPath" | ||
| } | ||
| } | ||
| catch { | ||
| Write-Error "Failed to grant permissions: $_" | ||
| return [PSCustomObject]@{ | ||
| Success = $false | ||
| ErrorMessage = $_.Exception.Message | ||
| NodeName = $env:COMPUTERNAME | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
There's a similarly named OLD function Grant-AdfsCertificatePermissionsOLD (lines 647-719) that should be removed if it's no longer needed. Clean up deprecated code before release.
| protected virtual void Dispose(bool disposing) | ||
| { | ||
| if (!_disposed) | ||
| { | ||
| if (disposing) | ||
| { | ||
| _primaryPsHelper = null; |
There was a problem hiding this comment.
The _logger field is set to null during disposal but this may cause issues if any methods are called after disposal. Consider adding a disposed check in methods that use _logger.
| ## Overview | ||
|
|
||
| TODO Overview is a required section | ||
|
|
There was a problem hiding this comment.
The Overview section contains "TODO Overview is a required section" instead of actual content. This placeholder text should be replaced with proper documentation before release.
| - **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 README contains "TODO Overview is a required section" as a placeholder. This must be replaced with actual content describing the IISU store type before merging.
| if (this.isADFSStore) throw new Exception("Remote ADFS stores are not supported."); | ||
|
|
There was a problem hiding this comment.
The conditional check for isADFSStore throws an exception for remote ADFS stores, but the store type configuration requires running as an agent on the primary ADFS server. Consider adding validation earlier in the process or providing a more informative error message that explains ADFS stores must run locally on the primary ADFS server.
| if (this.isADFSStore) throw new Exception("Remote ADFS stores are not supported."); | |
| bool isAdfsStore; | |
| try | |
| { | |
| isAdfsStore = this.isADFSStore; | |
| } | |
| catch (Exception ex) | |
| { | |
| throw new InvalidOperationException( | |
| "ADFS-backed certificate stores must run locally on the primary ADFS server. " + | |
| "Remote ADFS stores are not supported. Failed while evaluating ADFS store configuration.", | |
| ex); | |
| } | |
| if (isAdfsStore) | |
| { | |
| throw new InvalidOperationException( | |
| "ADFS-backed certificate stores must run locally on the primary ADFS server. " + | |
| "Remote ADFS stores are not supported."); | |
| } |
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):
Windows Server 2016 (IIS 10.0):
Windows Server 2019 (IIS 10.0.17763)
Windows Server 2022+ (IIS 10.0.20348+)