Skip to content

105314 dev branch for release 4 1 - #200

Open
rcpokorny wants to merge 38 commits into
release-4.1from
105314-Dev_Branch_For_Release_4_1
Open

rcpokorny wants to merge 38 commits into
release-4.1from
105314-Dev_Branch_For_Release_4_1

Conversation

@rcpokorny

Copy link
Copy Markdown
Collaborator

No description provided.

rcpokorny and others added 30 commits March 9, 2026 13:28
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>
* Fixed an issues when renewing ECC Certificates

* Update generated docs

* validating documentation for IIS/SSL Flags.

* Update generated docs

---------

Co-authored-by: Bob Pokorny <bpokorny@keyfactor.com>
Co-authored-by: Keyfactor <keyfactor@keyfactor.github.io>
* Fix problem only SQL Service was getting restarted while the other SQL Services would not restart.

* Update generated docs

* Potential fix for pull request finding

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

* Potential fix for pull request finding

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

* Update generated docs

* Delete local.runsettings

---------

Co-authored-by: Bob Pokorny <bpokorny@keyfactor.com>
Co-authored-by: Keyfactor <keyfactor@keyfactor.github.io>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
…e System.Security.Cryptography.XML package (#188)

Co-authored-by: Bob Pokorny <bpokorny@keyfactor.com>
Automates the customer's manual LDAPS certificate renewal workflow: staging
into the Personal store, then registering into the NTDS service certificate
store the LDAPS listener reads from. Each Domain Controller is managed as an
independent store (no ADFS-style fan-out), and both local-agent and remote
WinRM/JEA connections are supported since every operation stays local to the
target DC. Several NTDS-store mechanics (exact certutil syntax, JEA write
permissions, LDAPS pickup timing) are flagged as unverified pending lab
validation on a real Domain Controller - see docs/winldap-implementation-notes.md
and docs/winldap-ntds-validation.ps1.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
	modified:   IISU/ImplementedStoreTypes/WinLDAP/Inventory.cs
	modified:   IISU/ImplementedStoreTypes/WinLDAP/Management.cs
	modified:   IISU/PowerShell/Keyfactor.WinCert.LDAP/Keyfactor.WinCert.LDAP.psm1
	modified:   IISU/PowerShell/Keyfactor.WinCert.LDAP/Private/Get-NtdsServiceStoreCertificate.ps1
	deleted:    IISU/PowerShell/Keyfactor.WinCert.LDAP/Private/Invoke-CertUtilNtdsStore.ps1
	modified:   IISU/PowerShell/Keyfactor.WinCert.LDAP/Private/Remove-NtdsServiceStoreCertificate.ps1
	modified:   IISU/PowerShell/Keyfactor.WinCert.LDAP/Private/Set-NtdsServiceStoreCertificate.ps1
	modified:   IISU/PowerShell/Keyfactor.WinCert.LDAP/Public/Add-KeyfactorLdapsCertificate.ps1
	modified:   IISU/PowerShell/Keyfactor.WinCert.LDAP/Public/Remove-KeyfactorLdapsCertificate.ps1
	deleted:    IISU/PowerShell/Keyfactor.WinCert.LDAP/RoleCapabilities/Keyfactor.WinCert.LDAP.psrc
	modified:   docs/winldap-implementation-notes.md
	new file:   docs/winldap-module-validation.ps1
	modified:   docs/winldap-ntds-validation.ps1
	modified:   docsource/winldap.md
	modified:   IISU/ImplementedStoreTypes/WinLDAP/Inventory.cs
	modified:   IISU/ImplementedStoreTypes/WinLDAP/Management.cs
	modified:   IISU/PowerShell/Build/KeyfactorWinCert.pssc
	modified:   IISU/PowerShell/Keyfactor.WinCert.LDAP/Keyfactor.WinCert.LDAP.psm1
	new file:   IISU/PowerShell/Keyfactor.WinCert.LDAP/RoleCapabilities/Keyfactor.WinCert.LDAP.psrc
	modified:   docs/winldap-implementation-notes.md
	modified:   docs/winldap-module-validation.ps1
	modified:   docs/winldap-ntds-validation.ps1
	modified:   docsource/content.md
	modified:   docsource/winldap.md
Lab testing on a live DC showed removing only the NTDS registry entry
does not stop LDAPS from presenting the certificate, even after
restarting the NTDS service. Removing the Personal (My) store copy,
with no restart required, did stop it. Remove-KeyfactorLdapsCertificate
now removes from both locations, failing hard if the Personal-store
removal fails so the bug can't silently resurface. Also documents the
new shared-store caveat: this can now affect other services on the
same DC that reuse the same certificate.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Brings WinLDAP to parity with WinCert/IISU/WinSql. The shared
ClientPSCertStoreReEnrollment now imports the re-enrolled certificate
into the Personal store for WinLDAP (its NTDS\My store path isn't a
real Cert: provider path), then a new WinLdap binding case registers
it into the NTDS service store via the same eligibility-checked write
Add-KeyfactorLdapsCertificate uses. Matches WinSql's own ReEnrollment
precedent of not restarting the service or cleaning up a prior
certificate afterward.

Also adds docs/winldap-reset-test-state.ps1, a guided script for
resetting a lab DC's NTDS/Personal store state between test passes
without blindly clearing certificates that may be unrelated to testing.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Set-NtdsServiceStoreCertificate was rewritten during the September 1
lab validation to take -Certificate <X509Certificate2> instead of
-RawCertificateBytes <byte[]>. Add-KeyfactorLdapsCertificate already
called it correctly; Register-KeyfactorLdapsCertificate was written
afterward against the stale signature, causing the first live ODKG
test to fail with "A parameter cannot be found that matches parameter
name 'RawCertificateBytes'." after CSR generation/signing/import into
Personal all succeeded.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Adds a new store type, WinNetSH, for managing SSL certificates bound to
HTTP.sys via `netsh http sslcert` (e.g. the WinRM HTTPS listener, or any
other HTTP.sys-hosted service without its own binding mechanism).
Supports Inventory, Add, Remove, and Reenrollment (ODKG), following the
same binding-based pattern as WinIIS, including local-agent, remote
WinRM/SSH, and JEA connections.

Since netsh has no native PowerShell cmdlet, Keyfactor.WinCert.NetSH
shells out to netsh.exe and parses its text output. netsh http add
sslcert requires an AppId; when the entry parameter is left blank, this
store type reuses an existing binding's AppId across a renewal (rather
than silently changing it) or generates a new one for a brand-new
binding, and reports back whichever value was actually used.

Adds xUnit coverage for NetSHBindingInfo (alias parsing, entry-parameter
construction) - the first automated test coverage any PowerShell-backed
store type in this repo has had. The local-agent path (Inventory, Add,
renewal-AppId-reuse, SNI binding, Remove) was validated end-to-end
against real netsh sslcert bindings; JEA support has not yet been lab
validated and is flagged as the main open item in
docs/winnetsh-implementation-notes.md.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Bob Pokorny and others added 5 commits October 12, 2026 18:01
	new file:   azure-test-lab/README.md
	new file:   azure-test-lab/docs/01-prerequisites.md
	new file:   azure-test-lab/docs/02-phase1-deploy.md
	new file:   azure-test-lab/docs/03-manual-dns-cutover.md
	new file:   azure-test-lab/docs/04-phase2-deploy.md
	new file:   azure-test-lab/docs/05-adfs-farm-bootstrap.md
	new file:   azure-test-lab/docs/06-verification-runbook.md
	new file:   azure-test-lab/docs/07-teardown-and-redeploy.md
	new file:   azure-test-lab/modules/network.bicep
	new file:   azure-test-lab/modules/nsg.bicep
	new file:   azure-test-lab/modules/windows-vm.bicep
	new file:   azure-test-lab/phase1-foundation/main.bicep
	new file:   azure-test-lab/phase1-foundation/phase1.parameters.json
	new file:   azure-test-lab/phase1-foundation/scripts/New-ADForest.ps1
	new file:   azure-test-lab/phase2-member-servers/main.bicep
	new file:   azure-test-lab/phase2-member-servers/phase2.parameters.json
	new file:   azure-test-lab/phase2-member-servers/scripts/Bootstrap-AdfsPrimary.ps1
	new file:   azure-test-lab/phase2-member-servers/scripts/Bootstrap-AdfsSecondary.ps1
	new file:   azure-test-lab/phase2-member-servers/scripts/Confirm-SqlServerReady.ps1
	new file:   azure-test-lab/phase2-member-servers/scripts/Initialize-AdfsPrereqs.ps1
	new file:   azure-test-lab/phase2-member-servers/scripts/Install-DevBoxPrereqs.ps1
	new file:   azure-test-lab/phase2-member-servers/scripts/Install-IISRole.ps1
Merging PowerShell fix into latest working version
Copilot AI balanced review requested due to automatic review settings October 12, 2026 23:21

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Verified error-propagation, localization, rollback, concurrency, IPv6, and ADFS handoff defects can cause false success, outages, or failed deployments.

13 open findings
What changed in this PR

Adds WinLDAP and WinNetSH certificate store support for release 4.1, including inventory, management, reenrollment, JEA integration, documentation, and Azure test infrastructure.

Changes:

  • Implements LDAPS NTDS-store and HTTP.sys certificate management.
  • Extends reenrollment, manifests, packaging, and store-creation scripts.
  • Adds deployment documentation and an Azure validation lab.
File Description
WindowsCertStore.UnitTests/​NetSHBindingInfoTests.cs Tests NetSH binding parsing.
scripts/​store_types/​powershell/​kfutil_create_store_types.ps1 Creates new store types.
scripts/​store_types/​bash/​kfutil_create_store_types.sh Creates new store types.
IISU/​WindowsCertStore.csproj Packages new PowerShell modules.
IISU/​WinCertJobTypeBase.cs Adds binding-type enum values.
IISU/​PSHelper.cs Initializes local PowerShell module paths.
IISU/​PowerShell/​Keyfactor.WinCert.NetSH/​RoleCapabilities/​Keyfactor.WinCert.NetSH.psrc Defines NetSH JEA capabilities.
IISU/​PowerShell/​Keyfactor.WinCert.NetSH/​Public/​Remove-KeyfactorNetSHCertificateIfUnused.ps1 Removes unused certificates.
IISU/​PowerShell/​Keyfactor.WinCert.NetSH/​Public/​Remove-KeyfactorNetSHBinding.ps1 Removes HTTP.sys bindings.
IISU/​PowerShell/​Keyfactor.WinCert.NetSH/​Public/​New-KeyfactorNetSHBinding.ps1 Creates HTTP.sys bindings.
IISU/​PowerShell/​Keyfactor.WinCert.NetSH/​Public/​Get-KeyfactorNetSHBoundCertificates.ps1 Inventories HTTP.sys bindings.
IISU/​PowerShell/​Keyfactor.WinCert.NetSH/​Private/​Set-NetShSslCertBinding.ps1 Implements binding replacement.
IISU/​PowerShell/​Keyfactor.WinCert.NetSH/​Private/​Get-NetShSslCertBinding.ps1 Executes NetSH queries.
IISU/​PowerShell/​Keyfactor.WinCert.NetSH/​Private/​ConvertFrom-NetShSslCertOutput.ps1 Parses NetSH output.
IISU/​PowerShell/​Keyfactor.WinCert.NetSH/​Keyfactor.WinCert.NetSH.psm1 Loads and exports NetSH functions.
IISU/​PowerShell/​Keyfactor.WinCert.LDAP/​RoleCapabilities/​Keyfactor.WinCert.LDAP.psrc Defines WinLDAP JEA capabilities.
IISU/​PowerShell/​Keyfactor.WinCert.LDAP/​Public/​Remove-KeyfactorLdapsCertificate.ps1 Removes LDAPS certificates.
IISU/​PowerShell/​Keyfactor.WinCert.LDAP/​Public/​Register-KeyfactorLdapsCertificate.ps1 Registers reenrolled certificates.
IISU/​PowerShell/​Keyfactor.WinCert.LDAP/​Public/​Get-KeyfactorLdapCertificates.ps1 Inventories the NTDS store.
IISU/​PowerShell/​Keyfactor.WinCert.LDAP/​Private/​Test-LdapsCertificateEligibility.ps1 Validates LDAPS eligibility.
IISU/​PowerShell/​Keyfactor.WinCert.LDAP/​Private/​Set-NtdsServiceStoreCertificate.ps1 Writes the NTDS store.
IISU/​PowerShell/​Keyfactor.WinCert.LDAP/​Private/​Remove-NtdsServiceStoreCertificate.ps1 Deletes NTDS entries.
IISU/​PowerShell/​Keyfactor.WinCert.LDAP/​Private/​Get-NtdsServiceStoreCertificate.ps1 Reads NTDS entries.
IISU/​PowerShell/​Keyfactor.WinCert.LDAP/​Keyfactor.WinCert.LDAP.psm1 Loads and exports LDAP functions.
IISU/​PowerShell/​Build/​KeyfactorWinCert.pssc Documents new JEA roles.
IISU/​manifest.json Registers new extension classes.
IISU/​ImplementedStoreTypes/​WinNetSH/​WinNetSHBinding.cs Bridges NetSH binding commands.
IISU/​ImplementedStoreTypes/​WinNetSH/​ReEnrollment.cs Enables NetSH reenrollment.
IISU/​ImplementedStoreTypes/​WinNetSH/​NetSHCertificateInfo.cs Models NetSH inventory data.
IISU/​ImplementedStoreTypes/​WinNetSH/​NetSHBindingInfo.cs Models NetSH binding identity.
IISU/​ImplementedStoreTypes/​WinLDAP/​WinLdapCertificateInfo.cs Models LDAP inventory data.
IISU/​ImplementedStoreTypes/​WinLDAP/​WinLdapBinding.cs Registers LDAP certificates.
IISU/​ImplementedStoreTypes/​WinLDAP/​ReEnrollment.cs Enables LDAP reenrollment.
IISU/​ClientPSCertStoreReEnrollment.cs Routes new reenrollment flows.
docsource/​winnetsh.md Documents WinNetSH.
docsource/​winldap.md Documents WinLDAP.
docsource/​images/​WinNetSH-entry-parameters-store-type-dialog-ProviderName.svg NetSH configuration reference.
docsource/​images/​WinNetSH-entry-parameters-store-type-dialog-Port.svg NetSH configuration reference.
docsource/​images/​WinNetSH-entry-parameters-store-type-dialog-IPAddress.svg NetSH configuration reference.
docsource/​images/​WinNetSH-entry-parameters-store-type-dialog-HostName.svg NetSH configuration reference.
docsource/​images/​WinNetSH-entry-parameters-store-type-dialog-AppId.svg NetSH configuration reference.
docsource/​images/​WinNetSH-custom-field-WinRM Protocol-validation-options-dialog.svg NetSH configuration reference.
docsource/​images/​WinNetSH-custom-field-WinRM Protocol-dialog.svg NetSH configuration reference.
docsource/​images/​WinNetSH-custom-field-WinRM Port-validation-options-dialog.svg NetSH configuration reference.
docsource/​images/​WinNetSH-custom-field-WinRM Port-dialog.svg NetSH configuration reference.
docsource/​images/​WinNetSH-custom-field-spnwithport-validation-options-dialog.svg NetSH configuration reference.
docsource/​images/​WinNetSH-custom-field-spnwithport-dialog.svg NetSH configuration reference.
docsource/​images/​WinNetSH-custom-field-ServerUseSsl-validation-options-dialog.svg NetSH configuration reference.
docsource/​images/​WinNetSH-custom-field-ServerUseSsl-dialog.svg NetSH configuration reference.
docsource/​images/​WinNetSH-custom-field-JEAEndpointName-validation-options-dialog.svg NetSH configuration reference.
docsource/​images/​WinNetSH-custom-field-JEAEndpointName-dialog.svg NetSH configuration reference.
docsource/​images/​WinLDAP-entry-parameters-store-type-dialog.svg LDAP configuration reference.
docsource/​images/​WinLDAP-entry-parameters-store-type-dialog-ProviderName.svg LDAP configuration reference.
docsource/​images/​WinLDAP-custom-field-WinRM Protocol-validation-options-dialog.svg LDAP configuration reference.
docsource/​images/​WinLDAP-custom-field-WinRM Protocol-dialog.svg LDAP configuration reference.
docsource/​images/​WinLDAP-custom-field-WinRM Port-validation-options-dialog.svg LDAP configuration reference.
docsource/​images/​WinLDAP-custom-field-WinRM Port-dialog.svg LDAP configuration reference.
docsource/​images/​WinLDAP-custom-field-spnwithport-validation-options-dialog.svg LDAP configuration reference.
docsource/​images/​WinLDAP-custom-field-spnwithport-dialog.svg LDAP configuration reference.
docsource/​images/​WinLDAP-custom-field-ServerUseSsl-validation-options-dialog.svg LDAP configuration reference.
docsource/​images/​WinLDAP-custom-field-ServerUseSsl-dialog.svg LDAP configuration reference.
docsource/​images/​WinLDAP-custom-field-RestartService-validation-options-dialog.svg LDAP configuration reference.
docsource/​images/​WinLDAP-custom-field-RestartService-dialog.svg LDAP configuration reference.
docsource/​images/​WinLDAP-custom-field-JEAEndpointName-validation-options-dialog.svg LDAP configuration reference.
docsource/​images/​WinLDAP-custom-field-JEAEndpointName-dialog.svg LDAP configuration reference.
docs/​winldap-reset-test-state.ps1 Resets LDAP validation state.
CHANGELOG.md Documents release changes.
azure-test-lab/​phase2-member-servers/​scripts/​Install-IISRole.ps1 Provisions the IIS test server.
azure-test-lab/​phase2-member-servers/​scripts/​Install-DevBoxPrereqs.ps1 Provisions development tooling.
azure-test-lab/​phase2-member-servers/​scripts/​Initialize-AdfsPrereqs.ps1 Prepares ADFS prerequisites.
azure-test-lab/​phase2-member-servers/​scripts/​Confirm-SqlServerReady.ps1 Validates the SQL test server.
azure-test-lab/​phase2-member-servers/​phase2.parameters.json Supplies phase-two defaults.
azure-test-lab/​phase1-foundation/​scripts/​New-ADForest.ps1 Creates the test AD forest.
azure-test-lab/​phase1-foundation/​phase1.parameters.json Supplies phase-one defaults.
azure-test-lab/​phase1-foundation/​main.bicep Deploys the lab foundation.
azure-test-lab/​modules/​nsg.bicep Defines lab network security.
azure-test-lab/​modules/​network.bicep Defines the lab network.
azure-test-lab/​docs/​07-teardown-and-redeploy.md Documents lab teardown.
azure-test-lab/​docs/​06-verification-runbook.md Documents lab verification.
azure-test-lab/​docs/​05-adfs-farm-bootstrap.md Explains ADFS bootstrapping.
azure-test-lab/​docs/​04-phase2-deploy.md Documents phase-two deployment.
azure-test-lab/​docs/​03-manual-dns-cutover.md Documents DNS cutover.
azure-test-lab/​docs/​02-phase1-deploy.md Documents phase-one deployment.
azure-test-lab/​docs/​01-prerequisites.md Documents lab prerequisites.
.gitignore Excludes local secret parameter files.

🧠 Review effort: Balanced


💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread IISU/ImplementedStoreTypes/WinNetSH/NetSHBindingInfo.cs
Comment thread IISU/ImplementedStoreTypes/WinLDAP/WinLdapBinding.cs
Comment thread CHANGELOG.md
…ailures correctly'

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Copilot AI balanced review requested due to automatic review settings October 15, 2026 16:05
…umeration errors'

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
…on errors terminating'

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

@rcpokorny rcpokorny left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated WinLADP and WinSH

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Critical certificate-store deletion and privileged JEA exposure issues, plus unresolved correctness and lab reliability defects, block approval.

6 open findings
10 resolved since last review
Previously missed (7)

In code that hasn't changed since last review

Medium severity Use the machine DNS suffix instead of USERDNSDOMAIN

IISU/​PowerShell/​Keyfactor.WinCert.LDAP/​Private/​Test-LdapsCertificateEligibility.ps1:53

USERDNSDOMAIN describes the executing user's domain, not the target computer's DNS suffix. It can be absent for LocalSystem/virtual-account execution or refer to a different trusted domain, causing valid DC certificates to fail both Add and Reenrollment. Obtain the machine's primary DNS suffix locally instead; this does not require an AD bind or double hop.

Medium severity Exclude newlines when parsing multiple certificate SANs

IISU/​PowerShell/​Keyfactor.WinCert.LDAP/​Private/​Test-LdapsCertificateEligibility.ps1:65

Get-CertificateSAN uses Format(1), which renders multiple SAN entries on separate lines. This character class includes newlines, so two DNS entries become one captured name containing both lines. A valid certificate whose DC FQDN is in SAN but not CN is then rejected by Add and ReEnrollment. Exclude CR/LF from each captured DNS name.

Medium severity Avoid errors for already-absent Personal-store certificates

IISU/​PowerShell/​Keyfactor.WinCert.LDAP/​Public/​Remove-KeyfactorLdapsCertificate.ps1:79

The intended already-absent Personal-store case still fails the orchestrator job. Remove-KeyfactorCertificate writes to the error stream before throwing when the certificate is missing; catching that exception here does not remove the error record, and PSHelper.ExecutePowerShell throws when PS.HadErrors is true. Check for the certificate before calling the helper so this expected no-op does not emit an error.

Medium severity Preserve SSL policy settings during binding renewal

IISU/​PowerShell/​Keyfactor.WinCert.NetSH/​Private/​Set-NetShSslCertBinding.ps1:81

Recreating an existing binding with only these arguments resets all its other SSL settings to netsh defaults. A binding configured with clientcertnegotiation=enable, for example, loses that setting during certificate renewal, potentially breaking mutual-TLS clients. Preserve and reapply the existing binding's SSL policy settings, or use an in-place update on supported systems.

Medium severity Check Chocolatey exit codes during prerequisite installation

azure-test-lab/​phase2-member-servers/​scripts/​Install-DevBoxPrereqs.ps1:37

Windows PowerShell's ErrorActionPreference = 'Stop' does not turn native executable exit codes into exceptions. If any Chocolatey installation fails, this script continues and reports that prerequisites were installed, so the deployment can succeed with an unusable dev box. Check each Chocolatey exit code and throw on installation failures, handling reboot-required codes explicitly.

Low severity Include action-on-unmanage in the Phase 1 deployment command

azure-test-lab/​docs/​02-phase1-deploy.md:36

The recommended stack deployment command omits the required --action-on-unmanage argument, so Azure CLI rejects it before deploying Phase 1. Specify the intended unmanaged-resource behavior explicitly.

Low severity Include action-on-unmanage in the Phase 2 deployment command

azure-test-lab/​docs/​04-phase2-deploy.md:37

The recommended stack deployment command also needs --action-on-unmanage; without it Azure CLI rejects the Phase 2 invocation. Include the resource-management action alongside the deny settings.

🧠 Review effort: Balanced

Comment on lines +34 to +35
$cleanThumbprint = ($Thumbprint -replace '[^a-fA-F0-9]', '').ToUpper()
$regPath = "HKLM:\SOFTWARE\Microsoft\Cryptography\Services\$ServiceName\SystemCertificates\$StoreName\Certificates\$cleanThumbprint"

# netsh.exe is the only external command this module shells out to - there is no native
# PowerShell cmdlet for HTTP.sys SSL certificate bindings.
VisibleExternalCommands = @('C:\Windows\System32\netsh.exe')
}
else {
foreach ($thumb in $ThumbprintsToRemove) {
$clean = ($thumb -replace '[^a-fA-F0-9]', '').ToUpper()
Copilot AI balanced review requested due to automatic review settings October 15, 2026 16:15

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Unresolved certificate-deletion hazards, TLS binding regressions, privileged JEA exposure, and reliability issues block approval.

7 open findings
Previously missed (8)

In code that hasn't changed since last review

Medium severity Treat absent NTDS entries as successful removal

IISU/​PowerShell/​Keyfactor.WinCert.LDAP/​Private/​Remove-NtdsServiceStoreCertificate.ps1:43

[P2] Treat an already-absent NTDS entry as a successful removal. Remove-KeyfactorLdapsCertificate deletes this entry before removing the Personal-store copy; if that second step fails, retrying immediately returns this error and never retries the remaining cleanup. Returning success for absence allows retries to complete, while terminating actual registry-access errors prevents them from being mistaken for absence.

Medium severity Derive DNS suffix from the target computer

IISU/​PowerShell/​Keyfactor.WinCert.LDAP/​Private/​Test-LdapsCertificateEligibility.ps1:48

[P2] Derive the DNS suffix from the target computer, not the logged-on user. USERDNSDOMAIN describes the execution identity's domain and can be unset for LocalSystem/JEA virtual accounts or differ from the DC's domain for a trusted-domain account. Valid certificates then fail both Add and ReEnrollment despite sufficient store permissions. Read the machine's primary DNS suffix locally, for example from TCP/IP configuration, without introducing a directory bind.

This issue also appears on line 65 of the same file.

Medium severity Avoid errors for absent Personal-store certificates

IISU/​PowerShell/​Keyfactor.WinCert.LDAP/​Public/​Remove-KeyfactorLdapsCertificate.ps1:66

[P2] Check for an absent Personal-store copy before calling the shared removal function. Remove-KeyfactorCertificate writes an error record before throwing its 'not found' exception; catching that exception here does not remove the record. PSHelper.ExecutePowerShell checks HadErrors, so this supposedly successful no-op still fails the job after deleting the NTDS entry. Avoid emitting an error for the already-absent case.

Medium severity Normalize default HTTP.sys certificate store names

IISU/​PowerShell/​Keyfactor.WinCert.NetSH/​Public/​Get-KeyfactorNetSHBoundCertificates.ps1:31

[P2] Include bindings that use HTTP.sys's default certificate store. When certstorename is omitted, HTTP.sys defaults to MY and netsh can report 'Certificate Store Name : (null)'. Comparing that literal value with 'My' excludes valid existing bindings from My inventory. Normalize the default/null representation to My before applying the store filter.

Medium severity Clean up implicitly created OS disks

azure-test-lab/​docs/​07-teardown-and-redeploy.md:45

[P2] Account for the implicitly created OS disks in this cleanup procedure. windows-vm.bicep tags the VM but does not tag its managed OS disk or set its deleteOption to Delete. Deleting the VM therefore leaves an untagged detached disk that the second tag-filtered command cannot find, so this fallback leaves billable disks behind. Record and delete the VM disk IDs explicitly, or configure disk deletion with the VM.

Medium severity Check Chocolatey exit codes explicitly

azure-test-lab/​phase2-member-servers/​scripts/​Install-DevBoxPrereqs.ps1:28

In Windows PowerShell, ErrorActionPreference='Stop' does not make a nonzero Chocolatey exit code terminate the script. A failed Git installation therefore continues to the final success message and can leave deployment marked successful without Git. Check the native exit code explicitly.

Low severity Specify action-on-unmanage for Phase 1 deployment

azure-test-lab/​docs/​02-phase1-deploy.md:36

[P2] Supply the required action-on-unmanage option in this deployment-stack command. az stack group create requires it, so the recommended command exits with a missing-argument error instead of deploying Phase 1. Set it explicitly to the intended resource-management policy, consistent with the teardown instructions.

Low severity Specify action-on-unmanage for Phase 2 deployment

azure-test-lab/​docs/​04-phase2-deploy.md:37

[P2] Supply the required action-on-unmanage option in this deployment-stack command. az stack group create requires it, so the recommended command exits with a missing-argument error instead of deploying Phase 2. Set it explicitly to the intended resource-management policy, consistent with the teardown instructions.

🧠 Review effort: Balanced


Write-Information "[VERBOSE] Adding sslcert binding at '$bindingKey' (certhash=$Thumbprint, appid=$resolvedAppId, certstorename=$StoreName)"
$addKeyArg = if ($useHostname) { "hostnameport=$bindingKey" } else { "ipport=$bindingKey" }
$addOutput = & netsh http add sslcert $addKeyArg "certhash=$Thumbprint" "appid=$resolvedAppId" "certstorename=$StoreName" 2>&1
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants