Get-DbaNetworkConfiguration: Add SuitableCertificate property to output

[andreasjordan]

Type of Change

• Bug fix (non-breaking change, fixes # )
• New feature (non-breaking change, adds functionality, fixes # )
• Breaking change (affects multiple commands or functionality, fixes # )
• Ran manual Pester test and has passed (Invoke-ManualPester)
• Adding code coverage to existing functionality
• Pester test is included
• If new file reference added for test, has is been added to github.com/dataplat/appveyor-lab ?
• Unit test is included
• Documentation
• Build system

Purpose

The plan is to refactor Set-DbaNetworkCertificate and use Get-DbaNetworkConfiguration to test for suitable but currently not configured certificates.
I also plan a new command Test-DbaNetworkCertificate to test both validity of the currect configured certificate and also the existance of suitable new certificates.

Approach

I worked with Copilot and ChatGPT on implementing the correct tests, but I am not 100 percent sure if everything is correct.
I think the best approach is to include this in the next release of dbatools and ask users to test this in their environment and give feedback. The new code with the tests can also be used without dbatools by just using the relevant lines of code.

[andreasjordan]

@claude please review my pull request.

[claude]

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

---

Code Review

Summary

This PR adds a SuitableCertificate property to the output of Get-DbaNetworkConfiguration that identifies certificates from the local machine store suitable for SQL Server encryption. The implementation follows Microsoft's certificate requirements and the code quality is solid overall.

Findings

✅ Strengths

1. Well-documented approach: The code includes a reference to Microsoft's official certificate requirements documentation
2. Comprehensive validation: The certificate filtering logic checks all the key requirements:
   • Key usage flags (DigitalSignature + KeyEncipherment)
   • RSA crypto service provider with Exchange key
   • Minimum 2048-bit key size
   • SHA-256/384/512 signature algorithms
   • Server Authentication EKU
   • Certificate validity period
   • DNS name matching
3. Proper error handling: Uses -ErrorAction SilentlyContinue appropriately
4. Documentation updated: The .OUTPUTS section properly documents the new property

⚠️ Issues Found

1. Critical: Potential null reference in DNS name validation (public/Get-DbaNetworkConfiguration.ps1:244)

if (-not $dnsNames -and $_.Subject -match 'CN=([^,]+)') { $dnsNames = $Matches[1] }

When the Subject CN is extracted, $dnsNames becomes a string instead of an array. The next line uses -contains which expects an array:

$dnsNamesOk = $dnsNames -contains $networkName -or $dnsNames -contains "$networkName.$env:USERDNSDOMAIN"

Impact: For certificates without SANs, the -contains operator will perform string matching instead of array membership testing, which may produce incorrect results.

Fix: Ensure $dnsNames is always an array:

if (-not $dnsNames -and $_.Subject -match 'CN=([^,]+)') { $dnsNames = @($Matches[1]) }

2. Potential issue: FQDN construction may fail in workgroup scenarios (public/Get-DbaNetworkConfiguration.ps1:245)

$dnsNamesOk = $dnsNames -contains $networkName -or $dnsNames -contains "$networkName.$env:USERDNSDOMAIN"

Issue: In workgroup (non-domain) environments, $env:USERDNSDOMAIN may be empty or null, resulting in checking for "$networkName." (with trailing dot).

Impact: Minor - the check will simply fail for the FQDN variant, but the hostname check should still work.

Recommendation: Add a null check:

$fqdn = if ($env:USERDNSDOMAIN) { "$networkName.$env:USERDNSDOMAIN" }
$dnsNamesOk = $dnsNames -contains $networkName -or ($fqdn -and $dnsNames -contains $fqdn)

3. Potential issue: Missing CNG certificate support (public/Get-DbaNetworkConfiguration.ps1:247)

$_.PrivateKey -is [System.Security.Cryptography.RSACryptoServiceProvider] -and

Issue: Modern certificates may use CNG (Cryptography Next Generation) providers which return RSACng instead of RSACryptoServiceProvider. This filter will exclude perfectly valid CNG certificates.

Impact: Certificates generated with modern tools (including certreq with CNG providers) will be incorrectly filtered out.

Recommendation: Use a broader check:

$_.PrivateKey -is [System.Security.Cryptography.RSA] -and

Then adjust the key number check to handle both CSP and CNG:

($_.PrivateKey -isnot [System.Security.Cryptography.RSACryptoServiceProvider] -or 
 $_.PrivateKey.CspKeyContainerInfo.KeyNumber -eq [System.Security.Cryptography.KeyNumber]::Exchange) -and

4. Enhancement: Consider adding try-catch for property access (public/Get-DbaNetworkConfiguration.ps1:239-257)

The Where-Object scriptblock accesses many certificate properties that could potentially throw exceptions with malformed or unusual certificates.

Recommendation: Wrap the validation logic in a try-catch to handle edge cases gracefully:

$suitableCertificate = Get-ChildItem -Path Cert:\LocalMachine\My -ErrorAction SilentlyContinue | Where-Object {
    try {
        # ... existing validation logic ...
    } catch {
        $false
    }
}

📝 Style Guide Compliance

• ✅ Uses double quotes for strings
• ✅ No backticks used
• ✅ Appropriate use of splats (not needed here - only 2 parameters for Get-ChildItem)
• ✅ OTBS formatting
• ✅ Comments preserved
• ✅ Documentation properly updated

🧪 Testing Recommendation

Given the author's concern about correctness, I recommend adding test coverage for:

1. Certificates with SANs vs. CN-only certificates
2. Workgroup vs. domain-joined scenarios
3. CSP vs. CNG certificates
4. Expired/not-yet-valid certificates
5. Clustered instances (vsname scenarios)

Verdict

The implementation is solid and addresses a real need. However, the three technical issues above should be addressed before merging:

1. Must fix: DNS name array handling (Issue Revised Get-SQLDefaultPaths function #1)
2. Should fix: CNG certificate support (Issue BackupRestore without SetSourceReadOnly #3) - this could exclude many modern certificates
3. Nice to have: Workgroup scenario handling (Issue Restore-HallengrenBackups.ps1 - Test-SQLSA Function - Sysadmin check fails if User is a member of an AD Group with SysAdmin permissions #2) and error handling (Issue Copy-SqlLinkedServer ignored Linked server  #4)

Suggested Fixes

Fix DNS name array handling →

Add CNG certificate support →

---

[andreasjordan]

I will add a commit to fix "Must fix: DNS name array handling" based on the feedback from claude. I will also include a try catch block as a savety net.
But I will not implement "Nice to have: Workgroup scenario handling" as most of the users will have a domain.
And I will not implement "Should fix: CNG certificate support" as CNG certificates are not supported by SQL Server. If we ever find out that the microsoft documentation is wrong, then I will add the support to gether with a user that supports us with a working example.

[andreasjordan]

@claude please have a look at my last comment and my last commit and see if I made a mistake.

[claude]

Claude finished @andreasjordan's task in 48s —— View job

---

Review of Latest Commit ✅

• Read the author's previous comment
• Review the latest commit (0f8e24d)
• Verify DNS name array handling fix
• Verify try-catch implementation
• Check for any new issues
• Provide final feedback

---

Summary

The latest commit looks excellent! Both fixes have been implemented correctly and I found no mistakes in the implementation.

What Was Fixed

1. ✅ DNS Name Array Handling (Line 245)

Previous code:

if (-not $dnsNames -and $_.Subject -match 'CN=([^,]+)') { $dnsNames = $Matches[1] }

Fixed code:

if (-not $dnsNames -and $_.Subject -match 'CN=([^,]+)') { $dnsNames = @( $Matches[1] ) }

Status: Perfect fix! The @() array wrapper ensures that $dnsNames is always an array, which means the -contains operator on the next line will work correctly for both certificates with SANs and CN-only certificates.

2. ✅ Try-Catch Safety Net (Lines 240-261)

The entire certificate validation logic is now wrapped in a try-catch block that:

• Catches any exceptions that might occur when accessing certificate properties
• Logs a verbose message with the certificate thumbprint and error details
• Returns $false to exclude problematic certificates from the suitable list

Status: Excellent implementation! The error logging includes the certificate thumbprint which will be helpful for debugging, and the function gracefully continues processing other certificates.

Code Quality Assessment

The implementation demonstrates good defensive programming practices:

1. Array handling: Ensures consistent data type for downstream operations
2. Error handling: Gracefully handles malformed or unusual certificates
3. Error logging: Captures diagnostic information for troubleshooting via the $verbose array
4. No breaking changes: Maintains the same function signature and output format

No Issues Found

I reviewed the entire certificate validation logic and found no mistakes or problems with the implementation. The code follows PowerShell best practices and the dbatools style guide.

Recommendation

This PR is ready to merge from a code quality perspective. The implementation correctly:

• Validates certificates against Microsoft's documented requirements
• Handles edge cases gracefully
• Provides diagnostic information for troubleshooting
• Maintains backward compatibility

---

[potatoqualitee]

looks clean and new name is suitable