New-DbaComputerCertificate - Leave nothing behind on the computer it runs on

public/New-DbaComputerCertificate.ps1

@@ -20,6 +20,8 @@ function New-DbaComputerCertificate {
 
         The certificate is generated using AD's webserver SSL template on the client machine and pushed to the remote machine.
 
+        The command leaves nothing behind on the computer it runs on: the copy of a self-signed certificate that certreq puts into the intermediate CA store is removed right away, a request the CA did not answer is removed from the REQUEST store together with its key, and a certificate created for another computer is removed from the local store together with its key once it has been exported.
+
     .PARAMETER ComputerName
         Specifies the target computer or computers where the certificate will be created and installed. Defaults to localhost.
         For SQL Server clusters, specify each cluster node here and use ClusterInstanceName for the cluster's virtual name.
@@ -457,33 +459,70 @@ function New-DbaComputerCertificate {
                 Add-Content $certCfg "Critical=2.5.29.17"
 
                 if ($PScmdlet.ShouldProcess("local", "Creating certificate for $computer")) {
-                    Write-ProgressHelper -StepNumber ($stepCounter++) -Message "Running: certreq -new $certCfg $certCsr"
-                    $create = certreq -new $certCfg $certCsr
+                    Write-ProgressHelper -StepNumber ($stepCounter++) -Message "Running: certreq -q -new $certCfg $certCsr"
+                    $create = certreq -q -new $certCfg $certCsr
                 }
 
                 if ($SelfSigned) {
                     $serial = (($create -Split "Serial Number:" -Split "Subject")[2]).Trim() # D:
                     $storedCert = Get-ChildItem Cert:\LocalMachine\My -Recurse | Where-Object SerialNumber -eq $serial
 
+                    # certreq installs a self-signed certificate twice: with its key in LocalMachine\My, and without the key in
+                    # LocalMachine\CA, the intermediate CA store. The copy serves no purpose, a self-signed certificate is its own
+                    # root, and it stays behind when the certificate is removed later. So it goes right away.
+                    if ($storedCert) {
+                        $caStore = New-Object System.Security.Cryptography.X509Certificates.X509Store -ArgumentList "CA", "LocalMachine"
+                        $caStore.Open("ReadWrite")
+                        foreach ($caCopy in $caStore.Certificates.Find("FindByThumbprint", $storedCert.Thumbprint, $false)) {
+                            $caStore.Remove($caCopy)
+                        }
+                        $caStore.Close()
+                    }
+
                     if ($computer.IsLocalHost) {
                         $storedCert | Select-Object * | Select-DefaultView -Property FriendlyName, DnsNameList, Thumbprint, NotBefore, NotAfter, Subject, Issuer
                     }
                 } else {
                     if ($PScmdlet.ShouldProcess("local", "Submitting certificate request for $computer to $CaServer\$CaName")) {
-                        Write-ProgressHelper -StepNumber ($stepCounter++) -Message "certreq -submit -config `"$CaServer\$CaName`" -attrib $certTemplate $certCsr $certCrt $certPfx"
-                        $submit = certreq -submit -config "$CaServer\$CaName" -attrib $certTemplate $certCsr $certCrt $certPfx
+                        Write-ProgressHelper -StepNumber ($stepCounter++) -Message "certreq -q -submit -config `"$CaServer\$CaName`" -attrib $certTemplate $certCsr $certCrt $certPfx"
+                        $submit = certreq -q -submit -config "$CaServer\$CaName" -attrib $certTemplate $certCsr $certCrt $certPfx
                     }
 
                     if ($submit -match "ssued") {
-                        Write-ProgressHelper -StepNumber ($stepCounter++) -Message "certreq -accept -machine $certCrt"
-                        $null = certreq -accept -machine $certCrt
+                        Write-ProgressHelper -StepNumber ($stepCounter++) -Message "certreq -q -accept -machine $certCrt"
+                        $null = certreq -q -accept -machine $certCrt
                         $cert = New-Object System.Security.Cryptography.X509Certificates.X509Certificate2 ($certCrt, $null, [System.Security.Cryptography.X509Certificates.X509KeyStorageFlags]::DefaultKeySet)
                         $storedCert = Get-ChildItem "Cert:\$store\$folder" -Recurse | Where-Object { $_.Thumbprint -eq $cert.Thumbprint }
                     } elseif ($submit) {
                         Write-Message -Level Warning -Message "Something went wrong"
                         Write-Message -Level Warning -Message "$create"
                         Write-Message -Level Warning -Message "$submit"
-                        Stop-Function -Message "Failure when attempting to create the cert on $computer. Exception: $_" -Target $computer -Continue
+                        # The CA did not issue the certificate, so the pending request and its key would stay in
+                        # LocalMachine\REQUEST forever. They go with the failure.
+                        # Other requests may have been created in the meantime by someone else, so only the request of this
+                        # call goes: the one whose public key is in the request file certreq wrote. Every request has its own key.
+                        $pendingRequests = @()
+                        if (Test-Path -Path $certCsr) {
+                            $requestFileBase64 = (Get-Content -Path $certCsr | Where-Object { $PSItem -notmatch "^-----" }) -join ""
+                            $requestFileHex = [System.BitConverter]::ToString([System.Convert]::FromBase64String($requestFileBase64))
+                            $pendingRequests = @(Get-ChildItem -Path Cert:\LocalMachine\REQUEST -ErrorAction SilentlyContinue | Where-Object { $requestFileHex.Contains([System.BitConverter]::ToString($PSItem.PublicKey.EncodedKeyValue.RawData)) } | ForEach-Object { $PSItem.Thumbprint })
+                        }
+                        foreach ($pendingRequest in $pendingRequests) {
+                            Write-Message -Level Verbose -Message "Removing the pending request $pendingRequest and its key from LocalMachine\REQUEST"
+                            $splatRemoveRequest = @{
+                                Thumbprint      = $pendingRequest
+                                Folder          = "REQUEST"
+                                DeleteKey       = $true
+                                Confirm         = $false
+                                EnableException = $true
+                            }
+                            try {
+                                $null = Remove-DbaComputerCertificate @splatRemoveRequest
+                            } catch {
+                                Write-Message -Level Warning -Message "The pending request $pendingRequest could not be removed from LocalMachine\REQUEST: $PSItem"
+                            }
+                        }
+                        Stop-Function -Message "Failure when attempting to create the cert on $computer. $($submit | Select-Object -Last 1)" -Target $computer -Continue
                     }
 
                     if ($Computer.IsLocalHost) {
@@ -501,7 +540,23 @@ function New-DbaComputerCertificate {
                     }
 
                     if ($PScmdlet.ShouldProcess("local", "Removing cert from disk but keeping it in memory")) {
-                        $storedCert | Remove-Item
+                        # The certificate now lives in the PFX data and belongs to the target computer. Removing only the store entry
+                        # would leave its private key on this computer, so the key is deleted with it.
+                        $splatRemoveLocal = @{
+                            Thumbprint      = $storedCert.Thumbprint
+                            DeleteKey       = $true
+                            Confirm         = $false
+                            EnableException = $true
+                        }
+                        try {
+                            $localRemoval = Remove-DbaComputerCertificate @splatRemoveLocal
+                            if ($localRemoval.PrivateKey -ne "Deleted") {
+                                Write-Message -Level Warning -Message "The private key of the certificate $($storedCert.Thumbprint) is still on $env:COMPUTERNAME: $($localRemoval.PrivateKey)"
+                            }
+                        } catch {
+                            # The PFX data is there, so the import on the target still goes ahead.
+                            Write-Message -Level Warning -Message "The certificate $($storedCert.Thumbprint) could not be removed from LocalMachine\My on $env:COMPUTERNAME: $PSItem"
+                        }
                     }
 
                     if ($ClusterInstanceName) { $secondaryNode = $true }
@@ -541,7 +596,7 @@ function New-DbaComputerCertificate {
                 try {
                     Remove-Item -Force -Recurse $certDir -ErrorAction SilentlyContinue
                 } catch {
-                    Stop-Function "Isue removing files from $certDir" -Target $certDir -ErrorRecord $_
+                    Stop-Function -Message "Issue removing files from $certDir" -Target $certDir -ErrorRecord $PSItem
                 }
             }
             Write-ProgressHelper -Completed

tests/New-DbaComputerCertificate.Tests.ps1

@@ -149,6 +149,12 @@ Describe $CommandName -Tag IntegrationTests {
         It "Returns the right default one year expiry date" {
             $defaultCert.NotAfter -match ((Get-Date).Date).AddMonths(12) | Should -BeTrue
         }
+
+        It "Leaves no copy of the certificate in the intermediate CA store" {
+            # certreq installs a self-signed certificate a second time, without its key, in LocalMachine\CA. The command removes that copy.
+            Test-Path -Path "Cert:\LocalMachine\My\$($defaultCert.Thumbprint)" | Should -BeTrue
+            Test-Path -Path "Cert:\LocalMachine\CA\$($defaultCert.Thumbprint)" | Should -BeFalse
+        }
     }
 
     Context "Can generate a new certificate with custom settings" {
@@ -207,7 +213,13 @@ Describe $CommandName -Tag IntegrationTests {
             # For a remote computer the command exports a PFX and imports it on the target. The provider has to survive that transfer.
             # On CI the instance is local, so the transfer only happens against a lab whose instance runs on another computer.
             $computerName = ([DbaInstanceParameter]$TestConfig.InstanceSingle).ComputerName
+            $remoteIsLocal = ([DbaInstanceParameter]$TestConfig.InstanceSingle).IsLocalHost
+            # The key files of this computer before the certificate for the other computer is created here. After the export
+            # the certificate and its key have to be gone from this computer again.
+            $keyFolders = "$env:ProgramData\Microsoft\Crypto\RSA\MachineKeys", "$env:ProgramData\Microsoft\Crypto\Keys"
+            $keyFilesBefore = @((Get-ChildItem -Path $keyFolders -File).Name)
             $remoteKspCert = New-DbaComputerCertificate -ComputerName $computerName -SelfSigned -Provider "Microsoft Software Key Storage Provider"
+            $keyFilesAfter = @((Get-ChildItem -Path $keyFolders -File).Name)
             $readProvider = {
                 param ($Thumbprint)
                 $cert = Get-ChildItem -Path "Cert:\LocalMachine\My\$Thumbprint"
@@ -247,5 +259,130 @@ Describe $CommandName -Tag IntegrationTests {
         It "Keeps the Key Storage Provider key when the certificate is imported on a remote computer" {
             $remoteProvider | Should -Be "Microsoft Software Key Storage Provider"
         }
+
+        It "Leaves neither the certificate nor its key on this computer after the transfer to a remote computer" {
+            if ($remoteIsLocal) {
+                Set-ItResult -Skipped -Because "the instance runs on this computer, so the certificate stays here"
+            }
+            Test-Path -Path "Cert:\LocalMachine\My\$($remoteKspCert.Thumbprint)" | Should -BeFalse
+            Test-Path -Path "Cert:\LocalMachine\CA\$($remoteKspCert.Thumbprint)" | Should -BeFalse
+            $keyFilesAfter | Where-Object { $PSItem -notin $keyFilesBefore } | Should -BeNullOrEmpty
+        }
+    }
+
+    Context "Leaves nothing behind when the CA does not answer" {
+        BeforeAll {
+            $PSDefaultParameterValues["*-Dba*:EnableException"] = $true
+
+            # A request for a CA waits in LocalMachine\REQUEST with its key. When the CA cannot be reached the command removes
+            # the request again, so the store and the key folders have to look as before.
+            $requestFolders = "$env:ProgramData\Microsoft\Crypto\RSA\MachineKeys", "$env:ProgramData\Microsoft\Crypto\Keys"
+            $requestsBefore = @((Get-ChildItem -Path Cert:\LocalMachine\REQUEST).Thumbprint)
+            $requestKeyFilesBefore = @((Get-ChildItem -Path $requestFolders -File).Name)
+            $splatUnreachableCa = @{
+                CaServer        = "nosuchca.dbatools.invalid"
+                CaName          = "NoSuchCA"
+                WarningVariable = "unreachableCaWarning"
+                WarningAction   = "SilentlyContinue"
+                EnableException = $false
+            }
+            $unreachableCaResult = New-DbaComputerCertificate @splatUnreachableCa
+            $requestsAfter = @((Get-ChildItem -Path Cert:\LocalMachine\REQUEST).Thumbprint)
+            $requestKeyFilesAfter = @((Get-ChildItem -Path $requestFolders -File).Name)
+
+            $PSDefaultParameterValues.Remove("*-Dba*:EnableException")
+        }
+
+        AfterAll {
+            $PSDefaultParameterValues["*-Dba*:EnableException"] = $true
+            # In case the command left the request behind after all.
+            foreach ($leftover in ($requestsAfter | Where-Object { $PSItem -notin $requestsBefore })) {
+                Remove-DbaComputerCertificate -Thumbprint $leftover -Folder REQUEST -DeleteKey
+            }
+            $PSDefaultParameterValues.Remove("*-Dba*:EnableException")
+        }
+
+        It "Warns instead of returning a certificate" {
+            $unreachableCaResult | Should -BeNullOrEmpty
+            # The command writes several warnings on this path, the last one is the message of Stop-Function.
+            ($unreachableCaWarning -join " ") | Should -Match "Failure when attempting to create the cert"
+        }
+
+        It "Removes the pending request and its key again" {
+            $requestsAfter | Where-Object { $PSItem -notin $requestsBefore } | Should -BeNullOrEmpty
+            $requestKeyFilesAfter | Where-Object { $PSItem -notin $requestKeyFilesBefore } | Should -BeNullOrEmpty
+        }
+    }
+
+    Context "Keeps a request of someone else when the CA does not answer" {
+        BeforeAll {
+            $PSDefaultParameterValues["*-Dba*:EnableException"] = $true
+
+            # Another enrollment creates its own machine request while the command waits for the CA. Only the submission is
+            # intercepted to create that request at exactly this moment; certreq, the store and the keys are real.
+            $otherSubject = "CN=dbatoolsci_otherrequest_$(Get-Random)"
+            $otherInf = "$([System.IO.Path]::GetTempPath())dbatoolsci_otherrequest_$(Get-Random).inf"
+            $otherCsr = "$otherInf.csr"
+            $otherInfContent = @"
+[Version]
+Signature="`$Windows NT`$"
+[NewRequest]
+Subject = "$otherSubject"
+KeyLength = 2048
+Exportable = TRUE
+MachineKeySet = TRUE
+ProviderName = "Microsoft Software Key Storage Provider"
+KeyAlgorithm = RSA
+RequestType = PKCS10
+"@
+            Set-Content -Path $otherInf -Value $otherInfContent
+            $mockCertreq = [scriptblock]::Create(@"
+if (`$args -contains "-submit") {
+    `$null = certreq.exe -q -new "$otherInf" "$otherCsr"
+}
+certreq.exe @args
+"@)
+            Mock -ModuleName dbatools -CommandName certreq -MockWith $mockCertreq
+
+            $requestsBefore = @((Get-ChildItem -Path Cert:\LocalMachine\REQUEST).Thumbprint)
+            $splatUnreachableCa = @{
+                CaServer        = "nosuchca.dbatools.invalid"
+                CaName          = "NoSuchCA"
+                WarningVariable = "otherRequestWarning"
+                WarningAction   = "SilentlyContinue"
+                EnableException = $false
+            }
+            $otherRequestResult = New-DbaComputerCertificate @splatUnreachableCa
+            $newRequests = @(Get-ChildItem -Path Cert:\LocalMachine\REQUEST | Where-Object Thumbprint -notin $requestsBefore)
+            $otherRequest = $newRequests | Where-Object Subject -eq $otherSubject
+
+            $PSDefaultParameterValues.Remove("*-Dba*:EnableException")
+        }
+
+        AfterAll {
+            $PSDefaultParameterValues["*-Dba*:EnableException"] = $true
+            foreach ($leftover in $newRequests) {
+                Remove-DbaComputerCertificate -Thumbprint $leftover.Thumbprint -Folder REQUEST -DeleteKey
+            }
+            Remove-Item -Path $otherInf, $otherCsr -ErrorAction SilentlyContinue
+            $PSDefaultParameterValues.Remove("*-Dba*:EnableException")
+        }
+
+        It "Warns instead of returning a certificate" {
+            $otherRequestResult | Should -BeNullOrEmpty
+            ($otherRequestWarning -join " ") | Should -Match "Failure when attempting to create the cert"
+        }
+
+        It "Removes only its own request" {
+            $otherRequest | Should -Not -BeNullOrEmpty
+            $newRequests.Count | Should -Be 1
+        }
+
+        It "Keeps the key of the other request" {
+            $otherKey = [System.Security.Cryptography.X509Certificates.RSACertificateExtensions]::GetRSAPrivateKey($otherRequest)
+            $otherKey | Should -Not -BeNullOrEmpty
+            # Signing needs the key file itself, the certificate only points to it.
+            $otherKey.SignData([byte[]](1, 2, 3), [System.Security.Cryptography.HashAlgorithmName]::SHA256, [System.Security.Cryptography.RSASignaturePadding]::Pkcs1) | Should -Not -BeNullOrEmpty
+        }
     }
 }