Refactoring Set-DbaNetworkCertificate

[andreasjordan]

Implementing best practice in security is important, so we want to encourage the users to configure a certificate for their sql server instances. @claude, please have a look at the code of the command Set-DbaNetworkCertificate and give critical feedback on where the command needs refactoring to be very stable and to implement the current best practices.

[claude]

Claude finished @andreasjordan's task in 1m 41s —— View job

---

Code Review: Set-DbaNetworkCertificate

I've completed a thorough analysis of Set-DbaNetworkCertificate against dbatools best practices. Here are my findings:

---

Critical Issues Requiring Refactoring

1. PowerShell v3 Compatibility Violation ⚠️ CRITICAL

Location: Lines 188-192

The code uses $PSEdition which doesn't exist in PowerShell v3:

$keyName = switch ($PSEdition) {
    Core { $cert.PrivateKey.Key.UniqueName }
    Desktop { $cert.PrivateKey.CspKeyContainerInfo.UniqueKeyContainerName }
    default { $cert.PrivateKey.CspKeyContainerInfo.UniqueKeyContainerName }
}

Impact: This will fail in PowerShell v3 environments.

Recommendation: Use version detection that works in PS v3:

$keyName = if ($PSVersionTable.PSVersion.Major -ge 6) {
    $cert.PrivateKey.Key.UniqueName
} else {
    $cert.PrivateKey.CspKeyContainerInfo.UniqueKeyContainerName
}

2. Missing Error Handling in Script Block ⚠️ HIGH PRIORITY

Location: Lines 156-249

The remote script block has minimal error handling:

• Line 167: Get-ChildItem Cert:\LocalMachine -Recurse -ErrorAction Stop could fail but only stops within the script block
• Line 220-223: ACL operations lack error handling
• Line 226: Registry write lacks verification that permissions were actually granted

Impact: Silent failures or incomplete operations. If ACL operations fail, the registry is still updated, leaving SQL Server unable to read the certificate.

Recommendation:

• Wrap ACL operations in try/catch
• Verify ACL was successfully set before updating registry
• Return error objects instead of using Write-Warning and return (which swallows errors)

3. Weak Certificate Validation ⚠️ MEDIUM PRIORITY

Location: Lines 167-173

The certificate search is too broad and lacks important validations:

$cert = Get-ChildItem Cert:\LocalMachine -Recurse -ErrorAction Stop | Where-Object { $_.Thumbprint -eq $Thumbprint }

Issues:

• Searches all LocalMachine stores, not just LocalMachine\My where SQL Server certificates should be
• Doesn't validate that certificate has a private key before attempting to use it
• Doesn't validate certificate is valid (not expired, trusted chain)

Recommendation:

# Search only in LocalMachine\My (Personal store)
$cert = Get-ChildItem Cert:\LocalMachine\My -ErrorAction Stop | Where-Object { $_.Thumbprint -eq $Thumbprint }

if ($null -eq $cert) {
    Write-Warning "Certificate with thumbprint $Thumbprint does not exist in LocalMachine\My on $env:COMPUTERNAME"
    return
}

# Validate certificate has private key
if (-not $cert.HasPrivateKey) {
    Write-Warning "Certificate $Thumbprint does not have a private key on $env:COMPUTERNAME"
    return
}

# Validate certificate is not expired
if ($cert.NotAfter -lt (Get-Date)) {
    Write-Warning "Certificate $Thumbprint has expired (NotAfter: $($cert.NotAfter)) on $env:COMPUTERNAME"
    return
}

4. Incomplete Service SID Logic ⚠️ MEDIUM PRIORITY

Location: Lines 175-179

The Service SID logic only handles default and named instances:

$sqlSSID = "NT SERVICE\MSSQLSERVER"
if ($instanceName -ne "MSSQLSERVER") {
    $sqlSSID = "NT SERVICE\MSSQL$" + $instanceName
}

Issues:

• Doesn't account for clustered instances or custom service names
• Always grants permissions to both service account AND service SID, even though only one is needed depending on SQL Server version (2012+ uses virtual accounts)

Recommendation: Add validation that the service SID/account exists before granting permissions, and provide clear error messages if neither exists.

5. Registry Read Before Validation ⚠️ MEDIUM PRIORITY

Location: Line 165

The code reads the old thumbprint before validating that the certificate exists:

$oldThumbprint = (Get-ItemProperty -Path $regPath -Name Certificate).Certificate

Impact: If registry doesn't have a Certificate property, this will error and prevent setting the certificate the first time.

Recommendation: Use -ErrorAction SilentlyContinue or wrap in try/catch, since this is informational only.

---

Style and Best Practice Issues

6. Parameter Attributes Missing Modern Syntax

Location: Lines 89-92

Uses outdated ParameterSetName approach:

[parameter(Mandatory, ParameterSetName = "Certificate", ValueFromPipeline)]
[parameter(Mandatory, ParameterSetName = "Thumbprint", ValueFromPipelineByPropertyName)]

Recommendation: Per CLAUDE.md guidance, "Avoid ParameterSets - use Test-Bound instead with useful error messages". The current implementation at lines 102-105 already does this check, so the ParameterSets are redundant and should be removed.

7. Inconsistent Variable Casing

Location: Line 211

$KeyFullPath = $keyPath + $keyName  # Capital K

vs. line 193:

$keyFullPath = $keyPath + $keyName  # lowercase k

Recommendation: Use consistent casing ($keyFullPath throughout).

8. String Concatenation Instead of Join-Path

Location: Lines 187, 193, 195, 211

$keyPath = $env:ProgramData + "\Microsoft\Crypto\RSA\MachineKeys\"
$keyFullPath = $keyPath + $keyName

Recommendation: Use Join-Path for better cross-platform compatibility and cleaner code:

$keyPath = Join-Path $env:ProgramData "Microsoft\Crypto\RSA\MachineKeys"
$keyFullPath = Join-Path $keyPath $keyName

9. No Input Validation on Thumbprint Format

Location: Line 92

The $Thumbprint parameter accepts any string with no validation.

Recommendation: Add validation to ensure it's a valid SHA-1 thumbprint format:

[ValidatePattern('^[0-9A-Fa-f]{40}$')]
[string]$Thumbprint

10. Missing EnableException in Error Handling

Location: Lines 124-126, 259-261

Uses Stop-Function but doesn't respect -EnableException consistently in all error paths within the script block (which can't access Stop-Function).

Recommendation: The script block should return structured error objects that the main function can interpret and pass to Stop-Function with proper -EnableException handling.

---

Documentation Issues

11. Outdated Example Comments

Location: Line 134

$instanceName = $sqlwmi.DisplayName.Replace('SQL Server (', '').Replace(')', '') # Don't clown, I don't know regex :(

Recommendation: While humorous, this comment suggests uncertainty. The code works fine, but for a security-critical command, consider either using a regex pattern or removing the self-deprecating comment.

12. Missing .OUTPUTS Detail for Error Cases

Location: Lines 59-69

The .OUTPUTS section documents success cases but doesn't mention that $null might be returned on error, or that warnings might be the only output if remote operations fail.

Recommendation: Document all possible return scenarios including partial failures.

---

Security Concerns

13. No Validation of Service Restart Impact

Location: Lines 254-258

When -RestartService is used, there's no warning about connected users or active transactions.

Recommendation: Add a warning message when -RestartService is used:

if ($RestartService) {
    Write-Message -Level Warning -Message "Restarting SQL Server instance $instance - this will disconnect all users"
    $null = Restart-DbaService -SqlInstance $instance -Force
}

14. Thumbprint Case Sensitivity

Location: Line 226

The code converts thumbprint to lowercase for "SQL config compatibility", but this isn't validated or documented clearly.

Recommendation: Document why lowercase is required (SQL Server Configuration Manager displays/requires lowercase) and ensure input thumbprint comparison is case-insensitive.

---

Stability Improvements Needed

15. No Retry Logic for Remote Operations

Location: Line 253

Remote operations can fail transiently due to network issues, but there's no retry logic.

Recommendation: Consider adding retry logic for Invoke-Command2 calls, especially for production environments.

16. Missing Validation of Required Modules/Features

Location: Throughout

The command assumes:

• WMI/CIM access is available
• PowerShell remoting is enabled and accessible
• Required registry permissions exist

Recommendation: Add validation at the beginning to check prerequisites and fail fast with helpful error messages.

---

Summary and Prioritized Refactoring Plan

Critical (Do First):

1. Fix PowerShell v3 compatibility issue with $PSEdition
2. Add comprehensive error handling in script block
3. Improve certificate validation (private key, expiration, correct store)

High Priority:
4. Fix registry read error handling for first-time certificate setup
5. Add input validation for thumbprint format
6. Improve Service SID/account logic with existence checks

Medium Priority:
7. Remove redundant ParameterSets (use Test-Bound pattern)
8. Use Join-Path instead of string concatenation
9. Fix variable casing inconsistency
10. Add validation of service restart impact

Low Priority (Polish):
11. Update documentation for error scenarios
12. Consider adding retry logic for remote operations
13. Add prerequisite validation

---

The command is functional but has several stability and security concerns that should be addressed to make it production-ready for the important task of SSL certificate configuration.

[andreasjordan]

Very good feedback from claude. I agree with most parts, but also disagree on some.

• About 1. We should let go PowerShell v3 long term, maybe starting next major release. But I would also follow claude here.
• About 2. We should refactor that like I did some time ago in other commands. Will do that and maybe provide cloude with a "best practice".
• About 3. I'm not sure how to deal with certificates without privat key. I will investigate this as we have code for that case.
• About 4. I have a FCI for testing, so I will have a look at that. Maybe in that case there are more tests needed for the certificate to be working.
• About 5. I disagree here as the property is created at installation time so it should be there in every case. But we could check it anyway.
• About 8. The string part uses backslaches anyway, so where is the point in using Join-Path for only one part? And we are on windows anyway - so what is the point here?
• About 9. Good point, I like that.
• About 10. I think the code is ok. I think claude does not have information about how Stop-Function works.
• About 13. I disagree here as we don't do that in other commands.
• About 15. I disagree here as we generally don't implement retry logic.
• About 16. I'm not sure about this. This is a general topic that should be the same in all commands using Invoke-Command2.

@potatoqualitee Do you have additional feedback? I will start refactoring this command in a feq days.

[andreasjordan]

@claude please implement a new version Set-DbaNetworkCertificate which uses Test-DbaNetworkCertificate to very the certificate and follow all best practices.

[claude]

Claude finished @andreasjordan's task in 6m 47s —— View job • claude/issue-10125-20260311-0707 • Create PR ➔

---

Implementing Refactored Set-DbaNetworkCertificate

Todo List:

• Gather context (read existing files)
• Read Test-DbaNetworkCertificate to understand its interface
• Read style guides from .github/prompts/
• Implement refactored Set-DbaNetworkCertificate
• Update test file with new parameter
• Commit and push changes

---

Changes Made

New feature: Certificate validation via Test-DbaNetworkCertificate

Before setting a certificate, the command now calls Test-DbaNetworkCertificate to verify the certificate is suitable for SQL Server. If any check fails, the command stops with a detailed message listing which checks failed. Use -SkipCertificateValidation to bypass this.

# Now validates before setting (checks key usage, DNS names, private key, expiry, etc.)
Set-DbaNetworkCertificate -SqlInstance sql1 -Thumbprint 1223FB1ACBCA44D3EE9640F81B6BA14A92F3D6E2

# Bypass validation if needed
Set-DbaNetworkCertificate -SqlInstance sql1 -Thumbprint 1223FB1ACBCA44D3EE9640F81B6BA14A92F3D6E2 -SkipCertificateValidation

All best practice fixes applied:

Issue	Fix Applied
$PSEdition not available in PS v3	Replaced with $PSVersionTable.PSVersion.Major -ge 6
Redundant ParameterSets	Removed; using Test-Bound pattern
Certificate searched in all stores	Now searches only LocalMachine\My
No HasPrivateKey check in script block	Added explicit check
ACL operations lacked error handling	Wrapped in try/catch, returns on failure
Registry read could fail on first setup	Added -ErrorAction SilentlyContinue
Variable casing inconsistency $KeyFullPath	Fixed to $keyFullPath throughout
No thumbprint format validation	Added [ValidatePattern('^[0-9A-Fa-f]{40}$')]
foreach loop didn't continue after Stop-Function	Added explicit continue statements

Items intentionally not changed (per @andreasjordan's feedback):

• String concatenation instead of Join-Path (Windows-only, backslash paths)
• No retry logic for remote operations (not a dbatools pattern)
• No service restart warning (not done in other commands)

|