fix: Make Sign.SkipCertificateValidation actually skip validation

CHANGELOG.md

@@ -77,6 +77,25 @@
 
 ### Fixed
 
+- [**#193**](https://github.com/psake/PowerShellBuild/issues/193)
+  `$PSBPreference.Sign.SkipCertificateValidation` now does something. It was
+  read only by `psakeFile.ps1`, and even there `Get-PSBuildCertificate` consulted
+  it only for the `EnvVar` and `PfxFile` sources — the `Store` and `Thumbprint`
+  sources checked expiry and private key presence inside the filter that selects
+  the certificate, where the switch could not reach them. So the documented
+  escape hatch did nothing on two of the four sources for psake consumers, and
+  nothing at all for Invoke-Build consumers, because `IB.tasks.ps1` never passed
+  the setting to `Get-PSBuildCertificate` in either of its signing tasks. A
+  build with an expired store certificate failed with `NoCertificateFound`,
+  which points at a missing certificate rather than at the expiry that actually
+  caused it. `IB.tasks.ps1` now passes the setting, and for `Store` and
+  `Thumbprint` the relaxation is a fallback rather than a blanket bypass: an
+  unexpired certificate is still preferred whenever one exists, an expired one
+  is selected only when no unexpired one was found, and a warning is emitted
+  when that happens. `Thumbprint` still matches the requested thumbprint, and a
+  private key is still required in every case, because a certificate without one
+  cannot sign.
+
 - [**#191**](https://github.com/psake/PowerShellBuild/issues/191)
   The README's psake and Invoke-Build examples assigned
   `$PSBPreference.Test.ScriptAnalysisEnabled`, which is not a setting. The real
@@ -169,7 +188,7 @@
   that passed before may now correctly fail.
 - [**#96**](https://github.com/psake/PowerShellBuild/issues/96)
   `Test-PSBuildScriptAnalysis` no longer fails with a path-resolution error
   when `SettingsPath` is not supplied. An unsupplied path was forwarded to
   PSScriptAnalyzer as `-Settings ''`, which resolved against the current
   directory and threw before any analysis ran, so the function's own
   documented example could not run as written.

PowerShellBuild/IB.tasks.ps1

@@ -45,7 +45,7 @@
 
 
 
 $analyzePreReqs = {
     $result = $true
     if (-not $PSBPreference.Test.ScriptAnalysis.Enabled) {
         Write-Warning 'Script analysis is not enabled.'
@@ -59,7 +59,7 @@
 }
 
 # Synopsis: Execute PSScriptAnalyzer tests
 Task Analyze -If (. $analyzePreReqs) Build, {
     $analyzeParams = @{
         Path              = $PSBPreference.Build.ModuleOutDir
         SeverityThreshold = $PSBPreference.Test.ScriptAnalysis.FailBuildOnSeverityLevel
@@ -68,7 +68,7 @@
     Test-PSBuildScriptAnalysis @analyzeParams
 }
 
 $pesterPreReqs = {
     $result = $true
     if (-not $PSBPreference.Test.Enabled) {
         Write-Warning 'Pester testing is not enabled.'
@@ -86,7 +86,7 @@
 }
 
 # Synopsis: Execute Pester tests
 Task Pester -If (. $pesterPreReqs) Build, {
     $pesterParams = @{
         Path                         = $PSBPreference.Test.RootDir
         ModuleName                   = $PSBPreference.General.ModuleName
@@ -107,7 +107,7 @@
 
 
 
 $genMarkdownPreReqs = {
     $result = $true
     if (-not (Get-Module Microsoft.PowerShell.PlatyPS -ListAvailable)) {
         Write-Warning "Microsoft.PowerShell.PlatyPS module is not installed. Skipping [$($task.name)] task."
@@ -219,6 +219,7 @@
         CertStoreLocation         = $PSBPreference.Sign.CertStoreLocation
         CertificateEnvVar         = $PSBPreference.Sign.CertificateEnvVar
         CertificatePasswordEnvVar = $PSBPreference.Sign.CertificatePasswordEnvVar
+        SkipValidation            = $PSBPreference.Sign.SkipCertificateValidation
     }
     if ($PSBPreference.Sign.Thumbprint) {
         $certParams.Thumbprint = $PSBPreference.Sign.Thumbprint
@@ -294,6 +295,7 @@
         CertStoreLocation         = $PSBPreference.Sign.CertStoreLocation
         CertificateEnvVar         = $PSBPreference.Sign.CertificateEnvVar
         CertificatePasswordEnvVar = $PSBPreference.Sign.CertificatePasswordEnvVar
+        SkipValidation            = $PSBPreference.Sign.SkipCertificateValidation
     }
     if ($PSBPreference.Sign.Thumbprint) {
         $certParams.Thumbprint = $PSBPreference.Sign.Thumbprint

PowerShellBuild/Public/Get-PSBuildCertificate.ps1

@@ -42,25 +42,30 @@
     .PARAMETER CertificateEnvVar
         Name of the environment variable holding the Base64-encoded PFX certificate.
         Used by the EnvVar source and by Auto as the presence-detection key.
         Default: SIGNCERTIFICATE.
     .PARAMETER CertificatePasswordEnvVar
         Name of the environment variable holding the PFX password. Used by EnvVar source.
         Default: CERTIFICATEPASSWORD.
     .PARAMETER PfxFilePath
         File system path to a PFX/P12 certificate file. Required when CertificateSource is PfxFile.
     .PARAMETER PfxFilePassword
         Password for the PFX file as a SecureString. Used by PfxFile source.
     .PARAMETER SkipValidation
-        Skip validation checks (private key presence, expiration, Code Signing EKU) for certificates
-        loaded from EnvVar or PfxFile sources. Use with caution; invalid certificates will fail during
-        actual signing operations with less descriptive errors.
+        Relax the certificate validity checks. For the EnvVar and PfxFile sources, which load
+        exactly one certificate, this skips the private key, expiration, and Code Signing EKU
+        checks outright. For the Store and Thumbprint sources, which select one certificate out
+        of many, an unexpired certificate is still preferred whenever one exists; an expired
+        certificate is returned only when no unexpired one was found, and a warning is emitted
+        when that happens. A private key is required in every case, because a certificate
+        without one cannot sign. Use with caution; invalid certificates will fail during actual
+        signing operations with less descriptive errors.
     .OUTPUTS
         System.Security.Cryptography.X509Certificates.X509Certificate2
         Returns the resolved certificate, or $null if none was found (Store/Thumbprint sources).
     .EXAMPLE
         PS> $cert = Get-PSBuildCertificate
 
         Resolve automatically: use the SIGNCERTIFICATE env var when present, otherwise search
         the current user's certificate store.
     .EXAMPLE
         PS> $cert = Get-PSBuildCertificate -CertificateSource Store
@@ -77,7 +82,7 @@
         Decode a PFX certificate stored in a CI/CD secret environment variable.
     .EXAMPLE
         PS> $pass = Read-Host -Prompt 'Certificate password' -AsSecureString
         PS> $cert = Get-PSBuildCertificate -CertificateSource PfxFile -PfxFilePath './codesign.pfx' -PfxFilePassword $pass
 
         Load a code-signing certificate from a PFX file on disk.
     #>
@@ -129,9 +134,30 @@
             if ($null -ne $IsWindows -and -not $IsWindows) {
                 throw $LocalizedData.CertificateSourceStoreNotSupported
             }
-            $cert = Get-ChildItem -Path $CertStoreLocation -CodeSigningCert |
+            $candidateCertificate = Get-ChildItem -Path $CertStoreLocation -CodeSigningCert
+
+            # The store holds many certificates, so validity is part of how the right one is
+            # selected rather than a gate applied to a single loaded certificate. Prefer a valid
+            # certificate first, always.
+            $cert = $candidateCertificate |
                 Where-Object { $_.HasPrivateKey -and $_.NotAfter -gt (Get-Date) } |
                 Select-Object -First 1
+
+            # Only when the consumer explicitly opted out of validation does an expired
+            # certificate become acceptable, and only as a fallback, so a valid certificate is
+            # never passed over in favour of an expired one further down the store.
+            # HasPrivateKey stays required: a certificate without one cannot sign anything, so
+            # relaxing that check would buy nothing and only defer the failure to
+            # Set-AuthenticodeSignature with a less descriptive error.
+            if (-not $cert -and $SkipValidation) {
+                $cert = $candidateCertificate |
+                    Where-Object { $_.HasPrivateKey } |
+                    Select-Object -First 1
+                if ($cert) {
+                    Write-Warning ($LocalizedData.CertificateValidationRelaxed -f $cert.NotAfter, $cert.Subject)
+                }
+            }
+
             if ($cert) {
                 Write-Verbose ($LocalizedData.CertificateResolvedFromStore -f $CertStoreLocation, $cert.Subject)
             }
@@ -144,13 +170,34 @@
             # Normalize thumbprint input by removing whitespace for robust matching
             $normalizedThumbprint = ($Thumbprint -replace '\s', '')
 
-            $cert = Get-ChildItem -Path $CertStoreLocation -CodeSigningCert |
+            $candidateCertificate = Get-ChildItem -Path $CertStoreLocation -CodeSigningCert
+
+            # As with the Store source, validity is part of the selection. Prefer a valid
+            # certificate first, always.
+            $cert = $candidateCertificate |
                 Where-Object {
                     ($_.Thumbprint -replace '\s', '') -ieq $normalizedThumbprint -and
                     $_.HasPrivateKey -and
                     $_.NotAfter -gt (Get-Date)
                 } |
                 Select-Object -First 1
+
+            # The fallback relaxes only the expiry check. The thumbprint match is kept, because
+            # returning a certificate other than the one the consumer named would sign with a
+            # different identity than the build asked for, and HasPrivateKey is kept for the
+            # same reason it is kept in the Store source.
+            if (-not $cert -and $SkipValidation) {
+                $cert = $candidateCertificate |
+                    Where-Object {
+                        ($_.Thumbprint -replace '\s', '') -ieq $normalizedThumbprint -and
+                        $_.HasPrivateKey
+                    } |
+                    Select-Object -First 1
+                if ($cert) {
+                    Write-Warning ($LocalizedData.CertificateValidationRelaxed -f $cert.NotAfter, $cert.Subject)
+                }
+            }
+
             if ($cert) {
                 Write-Verbose ($LocalizedData.CertificateResolvedFromThumbprint -f $Thumbprint, $cert.Subject)
             }

PowerShellBuild/build.properties.ps1

@@ -186,11 +186,22 @@ $moduleVersion = (Import-PowerShellDataFile -Path $env:BHPSModuleManifest).Modul
         # Useful for Azure Key Vault, HSM, or other custom certificate providers.
         Certificate               = $null
 
-        # When true and using the Store or Thumbprint sources, skip the
-        # certificate validity check that ensures the certificate is not expired
-        # and has a private key. This is not recommended for production use but
-        # can be useful in CI environments where certificates are frequently
-        # renewed and updated.
+        # When true, relax the certificate validity checks. This is not
+        # recommended for production use but can be useful in CI environments
+        # where certificates are frequently renewed and rotated.
+        #
+        # The EnvVar and PfxFile sources load exactly one certificate, so the
+        # expiration and Code Signing EKU checks are skipped outright for them.
+        #
+        # The Store and Thumbprint sources select one certificate out of many,
+        # so the relaxation is a fallback rather than a blanket bypass: an
+        # unexpired certificate is preferred whenever one exists, and an expired
+        # one is returned only when no unexpired certificate was found. A
+        # warning is emitted when that happens, and the Thumbprint source still
+        # matches the requested thumbprint.
+        #
+        # A private key is required in every case, because a certificate without
+        # one cannot sign.
         SkipCertificateValidation = $false
 
         # RFC 3161 timestamp server URI embedded in Authenticode signatures.

PowerShellBuild/en-US/Messages.psd1

@@ -41,4 +41,5 @@ CertificateMissingPrivateKey=The resolved certificate does not have an accessibl
 CertificateExpired=The resolved certificate has expired (NotAfter: {0}). Code signing requires a valid, unexpired certificate. Subject=[{1}]
 CertificateMissingCodeSigningEku=The resolved certificate does not have the Code Signing Enhanced Key Usage (EKU: 1.3.6.1.5.5.7.3.3). Subject=[{0}]
 CertificateSourceStoreNotSupported=CertificateSource 'Store' is only supported on Windows platforms.
+CertificateValidationRelaxed=No unexpired code signing certificate was found, and validation was skipped, so an expired certificate was selected (NotAfter: {0}). Subject=[{1}]
 '@

tests/Get-PSBuildCertificate.tests.ps1

@@ -194,5 +194,146 @@ Describe 'Code Signing Functions' {
           Should -BeFalse
       }
     }
+
+    # The Store and Thumbprint sources select one certificate out of many, so validity is part
+    # of the selection rather than a gate applied to a single loaded certificate. SkipValidation
+    # therefore relaxes the selection only as a fallback: a valid certificate is preferred
+    # whenever one exists, and an expired one is accepted only when nothing valid was found.
+    # See psake/PowerShellBuild#193.
+    #
+    # These tests mock Get-ChildItem inside the module session state, which is the only way the
+    # mock reaches the call made by Get-PSBuildCertificate. They are Windows-only because the
+    # -CodeSigningCert dynamic parameter comes from the certificate provider, which exists only
+    # on Windows, and because the Store source throws on other platforms by design.
+    Context 'SkipValidation for store-backed sources' {
+
+      BeforeAll {
+        $script:validCertificate = [PSCustomObject]@{
+          Subject       = 'CN=Valid Test Certificate'
+          Thumbprint    = 'AAAA111122223333444455556666777788889999'
+          HasPrivateKey = $true
+          NotAfter      = (Get-Date).AddDays(30)
+        }
+        $script:expiredCertificate = [PSCustomObject]@{
+          Subject       = 'CN=Expired Test Certificate'
+          Thumbprint    = 'BBBB111122223333444455556666777788889999'
+          HasPrivateKey = $true
+          NotAfter      = (Get-Date).AddDays(-1)
+        }
+        $script:noPrivateKeyCertificate = [PSCustomObject]@{
+          Subject       = 'CN=No Private Key Test Certificate'
+          Thumbprint    = 'CCCC111122223333444455556666777788889999'
+          HasPrivateKey = $false
+          NotAfter      = (Get-Date).AddDays(30)
+        }
+      }
+
+      Context 'Store source' {
+        It 'Prefers a valid certificate over an expired one even when SkipValidation is set' -Skip:(-not $IsWindows) {
+          # The expired certificate is returned first so that a naive implementation, one that
+          # hoists the validity checks out of the selection filter, would pick it.
+          Mock -ModuleName PowerShellBuild -CommandName Get-ChildItem -MockWith {
+            $script:expiredCertificate
+            $script:validCertificate
+          }
+
+          $certificate = Get-PSBuildCertificate -CertificateSource Store -SkipValidation
+
+          $certificate.Subject | Should -Be 'CN=Valid Test Certificate'
+        }
+
+        It 'Returns an expired certificate when SkipValidation is set and nothing valid is available' -Skip:(-not $IsWindows) {
+          Mock -ModuleName PowerShellBuild -CommandName Get-ChildItem -MockWith {
+            $script:expiredCertificate
+          }
+
+          $certificate = Get-PSBuildCertificate -CertificateSource Store -SkipValidation -WarningAction SilentlyContinue
+
+          $certificate.Subject | Should -Be 'CN=Expired Test Certificate'
+        }
+
+        It 'Warns when SkipValidation causes an expired certificate to be selected' -Skip:(-not $IsWindows) {
+          Mock -ModuleName PowerShellBuild -CommandName Get-ChildItem -MockWith {
+            $script:expiredCertificate
+          }
+
+          Get-PSBuildCertificate -CertificateSource Store -SkipValidation `
+            -WarningVariable warningRecord -WarningAction SilentlyContinue | Out-Null
+
+          $warningRecord -join ' ' | Should -Match 'expired'
+        }
+
+        It 'Does not return an expired certificate when SkipValidation is not set' -Skip:(-not $IsWindows) {
+          Mock -ModuleName PowerShellBuild -CommandName Get-ChildItem -MockWith {
+            $script:expiredCertificate
+          }
+
+          $certificate = Get-PSBuildCertificate -CertificateSource Store
+
+          $certificate | Should -BeNullOrEmpty
+        }
+
+        It 'Does not return a certificate without a private key even when SkipValidation is set' -Skip:(-not $IsWindows) {
+          # A certificate with no private key cannot sign anything, so relaxing that check
+          # would only defer the failure to Set-AuthenticodeSignature with a worse message.
+          Mock -ModuleName PowerShellBuild -CommandName Get-ChildItem -MockWith {
+            $script:noPrivateKeyCertificate
+          }
+
+          $certificate = Get-PSBuildCertificate -CertificateSource Store -SkipValidation -WarningAction SilentlyContinue
+
+          $certificate | Should -BeNullOrEmpty
+        }
+      }
+
+      Context 'Thumbprint source' {
+        It 'Returns an expired certificate when SkipValidation is set and nothing valid is available' -Skip:(-not $IsWindows) {
+          Mock -ModuleName PowerShellBuild -CommandName Get-ChildItem -MockWith {
+            $script:expiredCertificate
+          }
+
+          $certificate = Get-PSBuildCertificate -CertificateSource Thumbprint `
+            -Thumbprint $script:expiredCertificate.Thumbprint -SkipValidation -WarningAction SilentlyContinue
+
+          $certificate.Subject | Should -Be 'CN=Expired Test Certificate'
+        }
+
+        It 'Still honours the requested thumbprint when SkipValidation relaxes the selection' -Skip:(-not $IsWindows) {
+          # A valid certificate is present, but it is not the one the consumer named. Returning
+          # it would sign with a different identity than the build asked for.
+          Mock -ModuleName PowerShellBuild -CommandName Get-ChildItem -MockWith {
+            $script:validCertificate
+            $script:expiredCertificate
+          }
+
+          $certificate = Get-PSBuildCertificate -CertificateSource Thumbprint `
+            -Thumbprint $script:expiredCertificate.Thumbprint -SkipValidation -WarningAction SilentlyContinue
+
+          $certificate.Thumbprint | Should -Be $script:expiredCertificate.Thumbprint
+        }
+
+        It 'Does not return an expired certificate when SkipValidation is not set' -Skip:(-not $IsWindows) {
+          Mock -ModuleName PowerShellBuild -CommandName Get-ChildItem -MockWith {
+            $script:expiredCertificate
+          }
+
+          $certificate = Get-PSBuildCertificate -CertificateSource Thumbprint `
+            -Thumbprint $script:expiredCertificate.Thumbprint
+
+          $certificate | Should -BeNullOrEmpty
+        }
+
+        It 'Does not return a certificate without a private key even when SkipValidation is set' -Skip:(-not $IsWindows) {
+          Mock -ModuleName PowerShellBuild -CommandName Get-ChildItem -MockWith {
+            $script:noPrivateKeyCertificate
+          }
+
+          $certificate = Get-PSBuildCertificate -CertificateSource Thumbprint `
+            -Thumbprint $script:noPrivateKeyCertificate.Thumbprint -SkipValidation -WarningAction SilentlyContinue
+
+          $certificate | Should -BeNullOrEmpty
+        }
+      }
+    }
   }
 }

tests/IBTasks.tests.ps1

@@ -1,4 +1,4 @@
-#requires -module InvokeBuild,Psake
+#requires -module InvokeBuild,Psake
 
 # Shared by both of the drift guards below, so each stays runnable on its own filter.
 BeforeAll {
@@ -177,3 +177,49 @@ Describe 'Settings documented in the README' {
         $undocumented -join ', ' | Should -BeNullOrEmpty
     }
 }
+
+Describe 'Signing settings referenced by the task files' {
+
+    # Both task files build a certificate parameter hashtable and splat it onto
+    # Get-PSBuildCertificate, and until psake/PowerShellBuild#193 nothing compared the two
+    # bodies. IB.tasks.ps1 never passed SkipValidation, so
+    # $PSBPreference.Sign.SkipCertificateValidation was dead for every Invoke-Build consumer
+    # while it worked for psake consumers. The task-name comparison above cannot see that,
+    # because both files define the same tasks.
+    #
+    # The comparison is scoped to the $PSBPreference.Sign settings rather than every
+    # $PSBPreference setting because the two files legitimately differ elsewhere: only
+    # IB.tasks.ps1 reads Build.Dependencies, and only psakeFile.ps1 uses the
+    # $PSB{TaskName}Dependency variables.
+
+    BeforeAll {
+        $script:moduleSourcePath = [IO.Path]::Combine(
+            (Split-Path -Path $PSScriptRoot -Parent), 'PowerShellBuild'
+        )
+
+        function script:Get-SignSettingName {
+            param([string]$FileName)
+
+            $taskFileContent = Get-Content -Path (
+                [IO.Path]::Combine($script:moduleSourcePath, $FileName)
+            ) -Raw
+
+            [regex]::Matches(
+                $taskFileContent, '\$PSBPreference\.Sign((?:\.[A-Za-z_][A-Za-z0-9_]*)+)'
+            ).ForEach({ $_.Groups[1].Value.TrimStart('.') }) | Sort-Object -Unique
+        }
+    }
+
+    It 'reads the same $PSBPreference.Sign settings in both task files' {
+        $psakeSetting = Get-SignSettingName -FileName 'psakeFile.ps1'
+        $invokeBuildSetting = Get-SignSettingName -FileName 'IB.tasks.ps1'
+
+        $psakeSetting | Should -Not -BeNullOrEmpty -Because 'the regex must still match something'
+
+        $missingFromInvokeBuild = $psakeSetting.Where({ $_ -notin $invokeBuildSetting })
+        $missingFromPsake = $invokeBuildSetting.Where({ $_ -notin $psakeSetting })
+
+        $missingFromInvokeBuild -join ', ' | Should -BeNullOrEmpty -Because 'IB.tasks.ps1 must honour every signing setting psakeFile.ps1 honours'
+        $missingFromPsake -join ', ' | Should -BeNullOrEmpty -Because 'psakeFile.ps1 must honour every signing setting IB.tasks.ps1 honours'
+    }
+}