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.
Fixed an issue with SSL flags
…he Windows Server.
…ng_Issues Fixed SSH Formatting issue
…Using_SSH Update generated docs
…e automatically added via Command.
There was a problem hiding this comment.
Pull request overview
This PR adds support for a new ADFS (Active Directory Federation Services) certificate store type to the Windows Certificate Orchestrator Extension version 3.0.0. The main changes include adding the WinAdfs store type for managing Service-Communications certificates on ADFS servers, removing the SAN (Subject Alternative Name) field from entry parameters for WinCert, WinIIS, and WinSql store types, and adding unit test infrastructure.
Key Changes:
- Added new ADFS Rotation Manager (WinAdfs) store type configuration
- Removed SAN field from entry parameters for existing store types
- Updated descriptions to indicate automatically created fields
- Added new documentation for WinAdfs functionality
- Added unit test projects and test cases
Reviewed changes
Copilot reviewed 39 out of 82 changed files in this pull request and generated 36 comments.
Show a summary per file
| File | Description |
|---|---|
| integration-manifest.json | Added WinAdfs store type configuration; removed SAN field from WinCert, WinIIS, and WinSql store types; updated field descriptions |
| docsource/winadfs.md | New documentation file for WinADFS store type functionality |
| docsource/content.md | Updated overview to include WinADFS and clarify supported job types |
| docsource/images/*.png | Added new screenshot images for WinSql entry parameter dialogs |
| WindowsCertStore.sln | Added unit test and integration test projects; removed inline ProjectSection items |
| WindowsCertStore.UnitTests/*.cs | New unit test files for SANs, PSHelper, Certificate utilities, and ADFS functionality |
| WindowsCertStore.UnitTests/*.csproj | New unit test project configuration |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| foreach (var error in PS.Streams.Error) | ||
| { | ||
| var errorMsg = error.ToString(); | ||
|
|
||
| // Execution policy messages are informational, not errors | ||
| if (errorMsg.Contains("execution policy successfully") || | ||
| errorMsg.Contains("setting is overridden")) | ||
| { | ||
| _logger.LogInformation($"Execution Policy Info: {errorMsg}"); | ||
| } | ||
| else | ||
| { | ||
| // Real error | ||
| _logger.LogError($"Execution Policy Error: {errorMsg}"); | ||
| throw new Exception($"Failed to set execution policy: {errorMsg}"); | ||
| } | ||
| } |
There was a problem hiding this comment.
This foreach loop immediately maps its iteration variable to another variable - consider mapping the sequence explicitly using '.Select(...)'.
| foreach (string subDir in Directory.GetDirectories(rootDirectory)) | ||
| { | ||
| string result = FindScriptsDirectory(subDir, directoryName); | ||
| if (!string.IsNullOrEmpty(result)) | ||
| { | ||
| return result; | ||
| } | ||
| } |
There was a problem hiding this comment.
This foreach loop immediately maps its iteration variable to another variable - consider mapping the sequence explicitly using '.Select(...)'.
| foreach (var entry in machines) | ||
| { | ||
| string machineName = entry["Machine"]; | ||
| string username = vault.GetSecret("Username"); | ||
| string password = vault.GetSecret("Password"); | ||
|
|
||
| yield return new object[] | ||
| { | ||
| new ClientConnection | ||
| { | ||
| Machine = machineName, | ||
| Username = username, | ||
| PrivateKey = password | ||
| } | ||
| }; | ||
| } |
There was a problem hiding this comment.
This foreach loop immediately maps its iteration variable to another variable - consider mapping the sequence explicitly using '.Select(...)'.
| private void InitializeLocalSessionOLD2() | ||
| { | ||
| _logger.LogTrace("Creating out-of-process Powershell Runspace."); | ||
| PowerShellProcessInstance psInstance = new PowerShellProcessInstance(new Version(5, 1), null, null, false); |
There was a problem hiding this comment.
Disposable 'PowerShellProcessInstance' is created but not disposed.
| private void InitializeLocalSessionOLD() | ||
| { | ||
| _logger.LogTrace("Creating out-of-process Powershell Runspace."); | ||
| PowerShellProcessInstance psInstance = new PowerShellProcessInstance(new Version(5, 1), null, null, false); |
There was a problem hiding this comment.
Disposable 'PowerShellProcessInstance' is created but not disposed.
| private ILogger _logger; | ||
|
|
||
| private PSHelper _primaryPsHelper; | ||
| private string _protocol; |
There was a problem hiding this comment.
Field '_protocol' can be 'readonly'.
| private string _protocol; | |
| private readonly string _protocol; |
| private PSHelper _primaryPsHelper; | ||
| private string _protocol; | ||
| private string _port; | ||
| private bool _useSPN; |
There was a problem hiding this comment.
Field '_useSPN' can be 'readonly'.
| private bool _useSPN; | |
| private readonly bool _useSPN; |
| private string serverPassword; | ||
|
|
||
| private bool isLocalMachine; | ||
| private bool isADFSStore = false; |
There was a problem hiding this comment.
Field 'isADFSStore' can be 'readonly'.
| private bool isADFSStore = false; | |
| private readonly bool isADFSStore = false; |
| public class SANsUnitTests | ||
| { | ||
|
|
||
| private ClientPSCertStoreReEnrollment enrollment = new ClientPSCertStoreReEnrollment(); |
There was a problem hiding this comment.
Field 'enrollment' can be 'readonly'.
| if (result.Success) | ||
| { | ||
| complete = new JobResult | ||
| { | ||
| Result = OrchestratorJobStatusJobResult.Success, | ||
| JobHistoryId = _jobHistoryID, | ||
| FailureMessage = $"Adfs Service Communication Certificate rotated successfully to thumbprint: {result.Thumbprint}" | ||
| }; | ||
| } | ||
| else | ||
| { | ||
| complete = new JobResult | ||
| { | ||
| Result = OrchestratorJobStatusJobResult.Failure, | ||
| JobHistoryId = _jobHistoryID, | ||
| FailureMessage = $"Adfs Service Communication Certificate rotation failed. {result.Message}" | ||
| }; | ||
| } |
There was a problem hiding this comment.
Both branches of this 'if' statement write to the same variable - consider using '?' to express intent better.
| if (result.Success) | |
| { | |
| complete = new JobResult | |
| { | |
| Result = OrchestratorJobStatusJobResult.Success, | |
| JobHistoryId = _jobHistoryID, | |
| FailureMessage = $"Adfs Service Communication Certificate rotated successfully to thumbprint: {result.Thumbprint}" | |
| }; | |
| } | |
| else | |
| { | |
| complete = new JobResult | |
| { | |
| Result = OrchestratorJobStatusJobResult.Failure, | |
| JobHistoryId = _jobHistoryID, | |
| FailureMessage = $"Adfs Service Communication Certificate rotation failed. {result.Message}" | |
| }; | |
| } | |
| complete = new JobResult | |
| { | |
| Result = result.Success | |
| ? OrchestratorJobStatusJobResult.Success | |
| : OrchestratorJobStatusJobResult.Failure, | |
| JobHistoryId = _jobHistoryID, | |
| FailureMessage = result.Success | |
| ? $"Adfs Service Communication Certificate rotated successfully to thumbprint: {result.Thumbprint}" | |
| : $"Adfs Service Communication Certificate rotation failed. {result.Message}" | |
| }; |
No description provided.