Refactor Set-DbaNetworkCertificate

[andreasjordan]

Complete refactoring of the command, closes #10125.

The code now uses Test-DbaNetworkCertificate which uses Get-DbaNetworkConfiguration and uses a similar structure like those commands.

New is the functionality to unset a certificate. This is useful if the currently configured certificate is no longer valid and sql server does not start anymore.

[andreasjordan]

@claude please perform a thorough code review.

[claude]

Claude finished @andreasjordan's task in 2m 55s —— View job

---

Code Review: Refactor Set-DbaNetworkCertificate

Tasks:

• Gather context and understand the request
• Read the changed files
• Analyze the code changes
• Post review feedback

---

This is a solid refactor overall. The new approach—delegating to Test-DbaNetworkCertificate for all pre-checks and using a remote scriptblock for the registry write—is cleaner and more consistent with the Get-/Set-DbaNetworkConfiguration pattern. The UnsetCertificate feature addresses a real operational need. Below are findings by severity.

---

Critical / Bugs

1. $Thumbprint is mutated inside the foreach loop, breaking multi-instance support

Set-DbaNetworkCertificate.ps1, lines 258 and 310

The parameter variable $Thumbprint is modified inside the foreach ($instance in $SqlInstance) loop in two places:

• $Thumbprint = $null (line 258, UnsetCertificate branch)
• $Thumbprint = $certTest.SuitableCertificates.Thumbprint (line 310, auto-select branch)

Because these mutations persist across loop iterations, when processing multiple instances:

# No -Thumbprint specified, no -UnsetCertificate
Set-DbaNetworkCertificate -SqlInstance instance1, instance2

• Instance 1 → auto-selects certA and sets $Thumbprint = "certA_thumbprint"
• Instance 2 → $Thumbprint is now "certA_thumbprint", so it falls into the elseif ($Thumbprint) branch instead of the else (auto-select) branch, and tries to apply certA to instance2—even if it isn't suitable there.

Fix: use a local variable inside the loop for the effective thumbprint, and do not mutate the parameter:

foreach ($instance in $SqlInstance) {
    $effectiveThumbprint = $Thumbprint  # local copy per-iteration

    ...

    if ($UnsetCertificate) {
        $effectiveThumbprint = $null
    } elseif ($effectiveThumbprint) {
        ...
    } else {
        $effectiveThumbprint = $certTest.SuitableCertificates.Thumbprint
    }

    # Use $effectiveThumbprint from here on
}

Fix this →

---

2. Tests capture warnings incorrectly — $WarnVar is never populated

Set-DbaNetworkCertificate.Tests.ps1, lines 54–76

Multiple It blocks reference $WarnVar, but none of the command invocations use -WarningVariable WarnVar. As a result:

• $WarnVar | Should -Match "No suitable certificate found" (line 57) — $WarnVar will always be $null, so this assertion will always fail.
• $WarnVar | Should -BeNullOrEmpty (lines 63, 69, 76) — $WarnVar will always be $null, so these will always pass (false positive).

The fix is to add -WarningVariable WarnVar to each invocation:

$result = Set-DbaNetworkCertificate -SqlInstance $TestConfig.InstanceRestart -RestartService `
    -WarningAction SilentlyContinue -WarningVariable WarnVar

Fix this →

---

Moderate

3. Error message doesn't distinguish set vs. unset on failure

Set-DbaNetworkCertificate.ps1, line 333

Stop-Function -Message "Failed to configure certificate $Thumbprint for instance $instance." ...

When $Thumbprint is $null (i.e., during an unset operation), this produces:

Failed to configure certificate for instance sql1.

The message should branch based on $UnsetCertificate:

if ($UnsetCertificate) {
    Stop-Function -Message "Failed to unset certificate for instance $instance." ...
} else {
    Stop-Function -Message "Failed to configure certificate $Thumbprint for instance $instance." ...
}

4. No validation when -UnsetCertificate is combined with -Certificate or -Thumbprint

Set-DbaNetworkCertificate.ps1, lines 220–228

If a user passes -Thumbprint someThumbprint -UnsetCertificate, the thumbprint format validation fires (which may not be intended) and then the thumbprint is silently ignored. A short-circuit with a clear message would improve UX:

if ($UnsetCertificate -and ($Thumbprint -or $Certificate)) {
    Stop-Function -Message "-UnsetCertificate cannot be used with -Thumbprint or -Certificate."
    return
}

---

Minor

5. Typo: "Failes" appears twice in the description

Set-DbaNetworkCertificate.ps1, lines 13 and 20

Failes if more than one or no suitable certificate is found.

Should be Fails.

6. $vsname is computed but never used in the scriptblock

Set-DbaNetworkCertificate.ps1, lines 152–161

$vsname is assigned in the scriptblock but never referenced again. If it's kept for pattern alignment with Get-DbaNetworkConfiguration, a brief comment explaining that would help. Otherwise it's dead code.

7. Original author removed entirely

Set-DbaNetworkCertificate.ps1, line 71

The original author (Chrissy LeMaire) was replaced rather than credited alongside the refactor. The dbatools convention for significant rewrites is typically to keep the original credit and add the refactor author:

Author: Chrissy LeMaire (@cl), netnerds.net | Refactored by Andreas Jordan (@andreasjordan)

8. Inconsistency between Resolve-DbaComputerName and $certTest.ComputerName

Set-DbaNetworkCertificate.ps1, line 325

$computerName for Invoke-Command2 is obtained via Resolve-DbaComputerName, but the output object uses $certTest.ComputerName (which comes from Get-DbaNetworkConfiguration). These should already be equivalent, but using $certTest.ComputerName for both would remove the extra call and reduce inconsistency.

---

Positive Highlights

• Reusing Test-DbaNetworkCertificate for pre-validation is excellent—it avoids duplicating connectivity, elevation, and WMI initialization logic.
• The remote scriptblock pattern (passing verbose + exception back as an object) matches the established pattern in Get-/Set-DbaNetworkConfiguration cleanly.
• The detailed failure check list (lines 287–296) gives users actionable information when a certificate fails validation—much better than a generic error.
• The UnsetCertificate feature is a useful addition, especially for the broken-certificate-on-startup scenario described in the PR.
• Test structure follows Pester v5 conventions well (header, BeforeAll/AfterAll, parameter validation block).

---

| Branch

[andreasjordan]

@claude based on your feedback I made some changes.

Please:

• Tell me if the changes to style.md help you to better review the tests
• Tell me if my changes cover all of your feedback
• Do another code review to find more abnormalities

[claude]

Claude finished @andreasjordan's task in 2m 47s —— View job

---

Claude finished @andreasjordan's task ——

---

Follow-up Review: Refactor Set-DbaNetworkCertificate

Tasks:

• Gather context
• Read the changed files (style.md, Set-DbaNetworkCertificate.ps1, tests)
• Assess style.md changes for test review usefulness
• Check if previous feedback items are addressed
• Full second-pass code review

---

On the style.md Changes

Yes, the addition is genuinely helpful. Knowing that $PSDefaultParameterValues['*-Dba*:WarningVariable'] = 'WarnVar' is set globally by $TestConfig.Defaults explains why the tests don't need -WarningVariable WarnVar on each call — my original feedback item 2 was incorrect, because the global default handles it. The style.md change makes this pattern discoverable and self-documenting.

One small nit: the example sentence at the end of the new section reads:

So $WarnVar can be used with WarnVar | Should -Be to test for warnings.

The WarnVar in the code snippet is missing its $ prefix — should read $WarnVar | Should -Be.

---

Status of Previous Feedback

#	Item	Status
1	$Thumbprint mutated inside loop	Partially fixed — see critical bug below
2	Tests don't capture warnings	Resolved by global defaults (my feedback was wrong)
3	Error message doesn't distinguish set vs. unset	✅ Fixed (lines 336–340)
4	No validation for -UnsetCertificate + -Certificate/-Thumbprint	✅ Fixed (lines 217–220)
5	Typo "Failes"	✅ Fixed
6	$vsname dead code	✅ Removed
7	Original author credit	✅ Fixed (line 71)
8	Resolve-DbaComputerName vs $certTest.ComputerName	✅ Fixed (line 329 uses $certTest.ComputerName)

---

Critical Bug (Regression from Fix #1)

Invoke-Command2 still passes $Thumbprint instead of $newThumbprint

Set-DbaNetworkCertificate.ps1, line 329

$result = Invoke-Command2 -ScriptBlock $scriptBlock -ArgumentList $instance, $Thumbprint ...

The fix for item #1 correctly introduced $newThumbprint as the local loop variable, but this line still passes the original $Thumbprint parameter to the remote scriptblock. This creates a regression in the auto-select path (no -Thumbprint specified, one suitable cert found):

• $Thumbprint = $null (no thumbprint was provided)
• $newThumbprint = "abc123..." (auto-selected suitable cert)
• Scriptblock receives $thumbprint = $args[1] = $null
• if ($thumbprint) evaluates to $false
• Scriptblock goes to the else (unset) branch and calls Set-ItemProperty ... -Value $null

The result: auto-select silently removes any configured certificate instead of setting the new one. This is the worst-case scenario — it appears to succeed (no error), returns a CertificateThumbprint of $newThumbprint (from the output object, line 361), but the actual registry value is null.

Fix:

$result = Invoke-Command2 -ScriptBlock $scriptBlock -ArgumentList $instance, $newThumbprint -ComputerName $($certTest.ComputerName) -Credential $Credential -ErrorAction Stop

Fix this →

---

Minor Issues

Warning message wording is misleading during unset

Set-DbaNetworkCertificate.ps1, line 353

Write-Message -Level Warning -Message "New certificate will not take effect until SQL Server services are restarted for $instance"

This message fires in all non-restart paths, including unset. When removing a certificate, saying "New certificate will not take effect" is inaccurate — there is no new certificate. A simple fix: use a more generic phrasing for both paths, or branch on $UnsetCertificate:

if ($UnsetCertificate) {
    Write-Message -Level Warning -Message "Certificate removal will not take effect until SQL Server services are restarted for $instance"
} else {
    Write-Message -Level Warning -Message "New certificate will not take effect until SQL Server services are restarted for $instance"
}

Similarly the $notes string on line 354 would benefit from the same distinction.

Test $WarnVar could be contaminated by Restart-DbaService warnings

Set-DbaNetworkCertificate.Tests.ps1, lines 63 and 69

$WarnVar | Should -BeNullOrEmpty

Since $WarnVar is set globally on all *-Dba* commands, the last dbatools call to run before the assertion determines the value of $WarnVar. In these two It blocks, the pipeline ends with Set-DbaNetworkCertificate, which calls Restart-DbaService internally. Any warnings emitted by Restart-DbaService would populate $WarnVar and cause the assertion to fail. This is a test fragility issue: it relies on Restart-DbaService never emitting warnings when it succeeds. It may be acceptable, but worth being aware of.

---

Positive Highlights

All significant feedback from the first pass has been addressed correctly, and the structure is much cleaner now. The $newThumbprint local variable approach is the right fix — it just needs the one-line change to Invoke-Command2 to complete it.

---

| Branch

[andreasjordan]

@claude I added more changes, so please look at the code again. Any open issues?

[claude]

Claude finished @andreasjordan's task in 2m 14s —— View job

---

Third-Pass Review: Refactor Set-DbaNetworkCertificate

Tasks:

• Gather context
• Read changed files
• Check remaining issues from previous review
• Full code review pass
• Post feedback

---

Status of Previous Issues

#	Item	Status
Critical (2nd pass)	Invoke-Command2 passing $Thumbprint instead of $newThumbprint	✅ Fixed (line 329)
Minor (2nd pass)	Warning message wording misleading during unset	✅ Fixed (lines 353–358)
Minor (2nd pass)	style.md typo (WarnVar missing $ prefix)	✅ Fixed
Minor (2nd pass)	$WarnVar contamination from Restart-DbaService	Still present (see below)

The critical regression is fully resolved. The code is in very good shape now.

---

One Remaining Bug

String interpolation typo in warning message

Set-DbaNetworkCertificate.ps1, line 358

Write-Message -Level Warning -Message "$notes for instance$instance"

The word "instance" is concatenated directly against $instance with no space or separator, producing output like:

New certificate will not take effect until SQL Server service is restarted for instancesql1\MSSQLSERVER

Fix — drop the redundant word "instance" since $instance already contains the full instance identifier:

Write-Message -Level Warning -Message "$notes for $instance"

Fix this →

---

Known Fragility (unchanged from previous review, low severity)

$WarnVar | Should -BeNullOrEmpty could be contaminated by Restart-DbaService warnings

Set-DbaNetworkCertificate.Tests.ps1, lines 63, 69, 76

Because $PSDefaultParameterValues['*-Dba*:WarningVariable'] = 'WarnVar' applies globally, Restart-DbaService called inside Set-DbaNetworkCertificate will also bind to that default, writing its warnings into $WarnVar. If Restart-DbaService emits a warning on a successful restart (e.g., a timing notice), these three assertions would fail spuriously. This is a test environment risk — no fix required if the CI environment is stable, but worth keeping in mind if these tests become flaky.

---

Summary

One minor string interpolation bug remains on line 358 — everything else from the previous reviews has been addressed correctly. Once that one-liner is fixed, this PR is clean.

---

| Branch

[andreasjordan]

I added the missing space and will not change the test.