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)

|

[rjleue]

@andreasjordan Previously, the Set-DbaNetworkCertificate function granted the SQL service account, which is being used, read permissions to the private key of the certificate. This is also mentioned in the function's description and is an official requirement documented by Microsoft. Unfortunately, this functionality has been removed with this PR. Could you please explain the reason behind this? Thank you

[andreasjordan]

Short answer: It is not needed. The running process gets all the rights that come from the account that service is started as (the "startname") plus all the rights that are granted to the service(account) itself ("servicename" / "NT Service\MSSQLSERVER"). As the startname can be changed and the sericename can not, it is better to grant the rights to that object that can not be changed. That way wou don't have to change rights when you change the startname.

Both Microsoft documentation and command documantation might be wrong. Can you point me to the exact sentences?