-
Notifications
You must be signed in to change notification settings - Fork 5
Old release 3.0 #171
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: release-3.0
Are you sure you want to change the base?
Old release 3.0 #171
Conversation
* 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 current SSL Flags (NOTE: Values greater than 4 are only supported in IIS 10 version 1809 and higher. The default value is 0): * 0 No SNI * 1 Use SNI * 2 Use Centralized SSL certificate store. * 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> Signed-off-by: Morgan Gangwere <morgan.gangwere@keyfactor.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adds support for the WinADFS (Active Directory Federation Services) store type to the Windows Certificate Orchestrator Extension. The changes include configuration updates, documentation, new test projects, and removal of the SAN field from WinSQL store type configurations.
- Addition of WinADFS store type for managing ADFS Service-Communications certificates
- Removal of SAN field from multiple store type configurations in the integration manifest
- Addition of unit test projects and test infrastructure
- Documentation updates for the new ADFS functionality
Reviewed changes
Copilot reviewed 39 out of 82 changed files in this pull request and generated 38 comments.
Show a summary per file
| File | Description |
|---|---|
| integration-manifest.json | Adds WinADFS store type configuration, removes SAN fields from WinCert and WinSQL store types, updates field descriptions to indicate auto-created fields |
| docsource/winadfs.md | New documentation file describing WinADFS store type requirements and configuration |
| docsource/images/*.png | New screenshot images for WinSQL store type dialogs |
| docsource/content.md | Updates overview to include WinADFS and clarifies job type support per extension |
| WindowsCertStore.sln | Adds unit test and integration test projects to solution |
| WindowsCertStore.UnitTests/*.cs | New unit test files for SANs, PowerShell helpers, certificates, and ADFS functionality |
| WindowsCertStore.UnitTests/WindowsCertStore.UnitTests.csproj | New test project configuration file |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -1,5 +1,6 @@ | |||
| ## Overview | |||
| The Windows Certificate Orchestrator Extension is a multi-purpose integration that can remotely manage certificates on a Windows Server's Local Machine Store. This extension currently manages certificates for the current store types: | |||
| * WinADFS - Rotates the Service-Communications certificate on the primary and secondary AFDS nodes | |||
Copilot
AI
Jan 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo: "AFDS" should be "ADFS" (Active Directory Federation Services).
| 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. |
Copilot
AI
Jan 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Grammatical error: "implements" should be "implement" to agree with the plural subject "other extensions".
| 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}"); | ||
| } | ||
| } |
Copilot
AI
Jan 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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; | ||
| } | ||
| } |
Copilot
AI
Jan 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 | ||
| } | ||
| }; | ||
| } |
Copilot
AI
Jan 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 | |
| } | |
| }; | |
| } | |
| return machines.Select(entry => | |
| { | |
| string machineName = entry["Machine"]; | |
| string username = vault.GetSecret("Username"); | |
| string password = vault.GetSecret("Password"); | |
| return new object[] | |
| { | |
| new ClientConnection | |
| { | |
| Machine = machineName, | |
| Username = username, | |
| PrivateKey = password | |
| } | |
| }; | |
| }); |
| private ILogger _logger; | ||
|
|
||
| private PSHelper _primaryPsHelper; | ||
| private string _protocol; |
Copilot
AI
Jan 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Field '_protocol' can be 'readonly'.
| private string _protocol; | |
| private readonly string _protocol; |
| private PSHelper _primaryPsHelper; | ||
| private string _protocol; | ||
| private string _port; | ||
| private bool _useSPN; |
Copilot
AI
Jan 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Field '_useSPN' can be 'readonly'.
| private bool _useSPN; | |
| private readonly bool _useSPN; |
| private string serverPassword; | ||
|
|
||
| private bool isLocalMachine; | ||
| private bool isADFSStore = false; |
Copilot
AI
Jan 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Field 'isADFSStore' can be 'readonly'.
| private bool isADFSStore = false; | |
| private readonly bool isADFSStore = false; |
| public class SANsUnitTests | ||
| { | ||
|
|
||
| private ClientPSCertStoreReEnrollment enrollment = new ClientPSCertStoreReEnrollment(); |
Copilot
AI
Jan 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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}" | ||
| }; | ||
| } |
Copilot
AI
Jan 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.