diff --git a/CHANGELOG.md b/CHANGELOG.md index 35e130a..a2e979c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -77,6 +77,25 @@ and this project adheres to [Semantic Versioning](http://semver.org/). ### 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 diff --git a/PowerShellBuild/IB.tasks.ps1 b/PowerShellBuild/IB.tasks.ps1 index 1142bfc..e299ffa 100644 --- a/PowerShellBuild/IB.tasks.ps1 +++ b/PowerShellBuild/IB.tasks.ps1 @@ -219,6 +219,7 @@ Task SignModule -If { 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 @@ Task SignCatalog -If { 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 diff --git a/PowerShellBuild/Public/Get-PSBuildCertificate.ps1 b/PowerShellBuild/Public/Get-PSBuildCertificate.ps1 index c8bdbec..2b97c49 100644 --- a/PowerShellBuild/Public/Get-PSBuildCertificate.ps1 +++ b/PowerShellBuild/Public/Get-PSBuildCertificate.ps1 @@ -51,9 +51,14 @@ function Get-PSBuildCertificate { .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). @@ -129,9 +134,30 @@ function Get-PSBuildCertificate { 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 @@ function Get-PSBuildCertificate { # 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) } diff --git a/PowerShellBuild/build.properties.ps1 b/PowerShellBuild/build.properties.ps1 index 2e8d24c..23a5ca9 100644 --- a/PowerShellBuild/build.properties.ps1 +++ b/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. diff --git a/PowerShellBuild/en-US/Messages.psd1 b/PowerShellBuild/en-US/Messages.psd1 index b2ef48e..4d141d6 100644 --- a/PowerShellBuild/en-US/Messages.psd1 +++ b/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}] '@ diff --git a/tests/Get-PSBuildCertificate.tests.ps1 b/tests/Get-PSBuildCertificate.tests.ps1 index bf9f9e6..317f657 100644 --- a/tests/Get-PSBuildCertificate.tests.ps1 +++ b/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 + } + } + } } } diff --git a/tests/IBTasks.tests.ps1 b/tests/IBTasks.tests.ps1 index 20b1816..5d7114e 100644 --- a/tests/IBTasks.tests.ps1 +++ b/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' + } +}