From baff21d2a043bcb6427978f845730ee652f2dc76 Mon Sep 17 00:00:00 2001 From: Trent Blackburn Date: Thu, 27 Aug 2026 20:56:35 -0400 Subject: [PATCH] docs: Correct claims the PR reviews found wrong Six independent reviews of #186, #187, #188, #190, #192, and #194 turned up documentation that describes behavior the code does not have. Each correction below is a claim a consumer could act on and be wrong. The private key claim was the serious one. build.properties.ps1, the Get-PSBuildCertificate docstring, and the changelog all said a private key is required in every case. It is not. SkipValidation gates the entire validation block for the EnvVar and PfxFile sources, and the private key check is the first thing inside it, so a certificate exported without its key is accepted and fails later in Set-AuthenticodeSignature with a far worse message. The reviewer demonstrated it with a public-only .cer rather than inferring it. The same edit had also dropped "private key" from the list of checks those two sources skip, which the docstring it replaced had correctly included. The guarantee is real only for Store and Thumbprint, where the private key is part of how the certificate is selected rather than a check layered on afterwards. The README described SkipCertificateValidation as applying only to EnvVar and PfxFile. That was accurate until #194 extended it to the store-backed sources as a fallback, and stale the moment #194 merged. Two reviewers flagged that neither pull request owned the reconciliation. Get-PSBuildHelpLocale was described as resolving the locale used for help generation. It returns the docs subdirectories that are help locales -- plural, and a classification rather than a resolution. An agent reading the old wording would come here to change the default locale, which is $PSBPreference.Help.DefaultLocale and lives elsewhere. The instructions said the shared CI workflow lints and then runs the tests. The lint and test jobs have no needs relationship and run in parallel, and cspell runs with strict disabled, so lint cannot gate anything. Someone debugging a red run would look at the wrong job. build.properties.ps1 still said Test.OutputFile is relative to the Pester test directory while shipping an absolute default -- the same contradiction #186 corrected in the README, left behind in the source. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01U1Jhu7fgTRJq7LK5MuKteE --- CHANGELOG.md | 9 ++++++--- .../Public/Get-PSBuildCertificate.ps1 | 9 +++++---- PowerShellBuild/build.properties.ps1 | 19 +++++++++++-------- README.md | 17 ++++++++++++++--- .../repository-specific.instructions.md | 7 ++++--- 5 files changed, 40 insertions(+), 21 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a2e979c..44ab9d3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -92,9 +92,12 @@ and this project adheres to [Semantic Versioning](http://semver.org/). `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. + when that happens. `Thumbprint` still matches the requested thumbprint, and + both store-backed sources still require a private key, because that is part of + how they select a certificate rather than a check layered on afterwards. Note + that `EnvVar` and `PfxFile` are different: the setting skips every check for + them, the private key check included, so a certificate exported without its + key is accepted here and fails later in `Set-AuthenticodeSignature` instead. - [**#191**](https://github.com/psake/PowerShellBuild/issues/191) The README's psake and Invoke-Build examples assigned diff --git a/PowerShellBuild/Public/Get-PSBuildCertificate.ps1 b/PowerShellBuild/Public/Get-PSBuildCertificate.ps1 index 2b97c49..69cab6e 100644 --- a/PowerShellBuild/Public/Get-PSBuildCertificate.ps1 +++ b/PowerShellBuild/Public/Get-PSBuildCertificate.ps1 @@ -1,4 +1,4 @@ -function Get-PSBuildCertificate { +function Get-PSBuildCertificate { <# .SYNOPSIS Resolves a code-signing X509Certificate2 from one of several common sources. @@ -56,9 +56,10 @@ function Get-PSBuildCertificate { 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. + when that happens. Those two sources always require a private key, because it is part of + how the certificate is selected rather than a check layered on afterwards; EnvVar and + PfxFile do not, since this skips their checks entirely. 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). diff --git a/PowerShellBuild/build.properties.ps1 b/PowerShellBuild/build.properties.ps1 index 23a5ca9..acd53cf 100644 --- a/PowerShellBuild/build.properties.ps1 +++ b/PowerShellBuild/build.properties.ps1 @@ -1,4 +1,4 @@ -# spell-checker:ignore PSGALLERY BHPS MAML +# spell-checker:ignore PSGALLERY BHPS MAML BuildHelpers\Set-BuildEnvironment -Force $outDir = [IO.Path]::Combine($env:BHProjectPath, 'Output') @@ -54,7 +54,8 @@ $moduleVersion = (Import-PowerShellDataFile -Path $env:BHPSModuleManifest).Modul # Specifies an output file path to send to Invoke-Pester's -OutputFile parameter. # This is typically used to write out test results so that they can be sent to a CI system - # This path is relative to the directory containing Pester tests + # The default below is absolute. A relative path resolves against the directory + # containing Pester tests, because Test-PSBuildPester pushes into it before running. OutputFile = [IO.Path]::Combine($env:BHProjectPath, 'testResults.xml') # Specifies the test output format to use when the TestOutputFile property is given @@ -190,18 +191,20 @@ $moduleVersion = (Import-PowerShellDataFile -Path $env:BHPSModuleManifest).Modul # 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 EnvVar and PfxFile sources load exactly one certificate, so every + # check is skipped outright for them -- the private key check included. + # A certificate without a private key cannot sign, so setting this for + # those two sources defers that failure to Set-AuthenticodeSignature, + # which reports it far less clearly. # # 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. + # matches the requested thumbprint. These two sources always require a + # private key, because it is part of how the certificate is selected + # rather than a check layered on afterwards. SkipCertificateValidation = $false # RFC 3161 timestamp server URI embedded in Authenticode signatures. diff --git a/README.md b/README.md index 9d1f53e..a66b930 100644 --- a/README.md +++ b/README.md @@ -138,7 +138,7 @@ match your environment. | $PSBPreference.Sign.PfxFilePath | `$null` | File system path to a PFX/P12 certificate file. Required by the `PfxFile` certificate source. | | $PSBPreference.Sign.PfxFilePassword | `$null` | Password for the PFX/P12 file as a `SecureString`. Used by the `PfxFile` certificate source. | | $PSBPreference.Sign.Certificate | `$null` | A pre-resolved `X509Certificate2` object to sign with. When set, `CertificateSource` is ignored, which suits Azure Key Vault, an HSM, or another custom provider. | -| $PSBPreference.Sign.SkipCertificateValidation | `$false` | Skip the private key, expiration, and Code Signing EKU checks made on certificates loaded by the `EnvVar` and `PfxFile` sources. Not recommended in production. | +| $PSBPreference.Sign.SkipCertificateValidation | `$false` | Relax certificate validity checking. Skips every check for `EnvVar` and `PfxFile`, private key included. See [Code signing](#code-signing). | | $PSBPreference.Sign.TimestampServer | `http://timestamp.digicert.com` | RFC 3161 timestamp server URI embedded in the signature so that it stays valid after the certificate expires. | | $PSBPreference.Sign.HashAlgorithm | `SHA256` | Authenticode hash algorithm. Valid values are `SHA256`, `SHA384`, `SHA512`, and `SHA1`. `SHA1` is deprecated. | | $PSBPreference.Sign.FilesToSign | `@('*.psd1', '*.psm1', '*.ps1')` | Glob patterns of file names to sign, searched recursively under the module output directory. | @@ -184,10 +184,14 @@ Where the code-signing certificate comes from is controlled by `$PSBPreference.Sign.CertificateSource`: - `Store` selects the first valid, unexpired code-signing certificate that has a - private key from `$PSBPreference.Sign.CertStoreLocation`. + private key from `$PSBPreference.Sign.CertStoreLocation`. With + `$PSBPreference.Sign.SkipCertificateValidation` set, an expired certificate is + used as a fallback when no unexpired one is found, and a warning says so; a + private key is always required, because it is part of the selection. - `Thumbprint` selects a specific certificate from that same store by `$PSBPreference.Sign.Thumbprint`, which is what you want when more than one - code-signing certificate is installed. + code-signing certificate is installed. The same expired-certificate fallback + applies, and the requested thumbprint is still matched. - `EnvVar` decodes a Base64-encoded PFX from the environment variable named by `$PSBPreference.Sign.CertificateEnvVar`, optionally decrypting it with the password in the variable named by @@ -196,6 +200,13 @@ Where the code-signing certificate comes from is controlled by held as a masked secret. - `PfxFile` loads a PFX/P12 file from `$PSBPreference.Sign.PfxFilePath` using `$PSBPreference.Sign.PfxFilePassword`. + +`EnvVar` and `PfxFile` load exactly one certificate rather than choosing from +many, so `$PSBPreference.Sign.SkipCertificateValidation` skips their checks +outright — expiry, Code Signing EKU, **and the private key**. A certificate +exported without its private key is accepted here and fails later in +`Set-AuthenticodeSignature`, with a much less helpful message. Prefer leaving +validation on for these two sources. - `Auto`, the default, uses `EnvVar` when the certificate environment variable is populated and falls back to `Store` when it is not. One build script can therefore sign with the developer's own certificate locally and with the diff --git a/instructions/repository-specific.instructions.md b/instructions/repository-specific.instructions.md index 70dd732..683a1d1 100644 --- a/instructions/repository-specific.instructions.md +++ b/instructions/repository-specific.instructions.md @@ -141,7 +141,7 @@ pattern — keep new public functions consistent with this): Private helpers in `PowerShellBuild/Private/`: - `Remove-ExcludedItem` — filters file system items by regular expression patterns during builds -- `Get-PSBuildHelpLocale` — resolves the locale used for help generation +- `Get-PSBuildHelpLocale` — classifies which docs subdirectories are help locales ### Invoke-Build alias @@ -258,7 +258,7 @@ own floor is **Pester 6.0.0** as of psake/PowerShellBuild#172, matching what CI | `Clear-PSBuildOutputFolder.tests.ps1` | Output directory removal | | `Fixtures.tests.ps1` | The shared test fixture helpers themselves | | `Get-PSBuildCertificate.tests.ps1` | Signing certificate resolution | -| `Get-PSBuildHelpLocale.tests.ps1` | Help locale resolution | +| `Get-PSBuildHelpLocale.tests.ps1` | Which docs subdirectories count as help locales | | `Help.tests.ps1` | Help documentation completeness | | `IBTasks.tests.ps1` | Invoke-Build task definitions and the settings they reference | | `Initialize-PSBuild.tests.ps1` | Build environment initialization | @@ -280,7 +280,8 @@ Supporting files: `tests/MetaFixers.psm1` (helpers for `Meta.tests.ps1`) and `te - The workflow itself defines no jobs of its own — it delegates to the psake organization's shared workflow, `psake/.github/.github/workflows/ModuleCI.yml@main`. Change the shared workflow, not `test.yml`, to change what CI runs -- The shared workflow lints (cspell plus PSScriptAnalyzer fix suggestions), then runs +- The shared workflow lints (cspell plus PSScriptAnalyzer fix suggestions) and, in + parallel, runs `./build.ps1 -Task Test -Bootstrap` across a `ubuntu-latest`, `windows-latest`, `macOS-latest` matrix on PowerShell 7 and again on Windows PowerShell 5.1, and publishes the test results