Harden Windows signing verification in CI by ashfame · Pull Request #28 · WordPress/experimental-wp-dev-env

ashfame · 2026-07-06T21:00:15Z

Stacked on #27. Follow-up hardening from the PR 21 Windows signing review.

This PR keeps the Windows Azure Trusted Signing migration, but makes CI fail closed around the signed artifact and the downloaded signing tools:

wraps native commands in the Windows Buildkite step so failed build commands cannot be masked by later verification commands
moves Windows artifact signature verification into .buildkite/commands/verify_windows_signature.ps1
fails when no dist/*.exe artifact is present
requires WINDOWS_EXPECTED_SIGNER_SUBJECT and verifies the Authenticode signer subject after signtool verify /pa /v
optionally checks WINDOWS_EXPECTED_SIGNER_ISSUER when set
verifies downloaded SIGNTOOL_PATH and AZURE_CODE_SIGNING_DLIB either by SHA256 pins (WINDOWS_SIGNTOOL_SHA256 / AZURE_CODE_SIGNING_DLIB_SHA256) or by valid Microsoft Authenticode signature
adds a Buildkite Tests step so the node:test coverage added in PR 21 actually runs in CI

CI setup note: before this lands, the Windows queue needs WINDOWS_EXPECTED_SIGNER_SUBJECT configured to a stable substring of the expected WordPress/Automattic signing certificate subject. The SHA256 env vars are optional but preferable if the downloaded tool versions are meant to be pinned exactly.

Copilot

Pull request overview

This PR hardens the Windows signing verification flow in Buildkite CI by making the Windows build step fail closed (both for produced artifacts and the downloaded signing tools) and by ensuring the Node test suite actually runs in CI.

Changes:

Wraps native command execution in the Windows Buildkite step to prevent non-zero exit codes from being masked.
Moves Windows artifact signature verification into a dedicated PowerShell script that enforces presence of dist\*.exe and verifies expected signer subject (and optional issuer).
Adds a Buildkite “Tests” step that runs npm test on the default queue.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File	Description
`.buildkite/pipeline.yml`	Hardens Windows step execution semantics and adds a CI test step.
`.buildkite/commands/verify_windows_signature.ps1`	Adds fail-closed Authenticode verification for Windows artifacts, including signer identity checks.
`.buildkite/commands/setup_windows_code_signing.ps1`	Verifies integrity of downloaded signing tools via SHA256 pinning or Microsoft Authenticode signature validation.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

mokagio

Thanks for this PR as well.

As in #27, I'm cursious whether this is a refinement motivated by a failure that was encountered or by caution.

It seems that Buildkite is not reporting the build status in the commits and PR checks, which is a problem we'll need to address.

In the meantime, I can confirm the new tests step passes:

Unfortunately, the new code in the Windows flow fails to parse:

[2026-07-06T21:40:49Z] At line:5 char:27
[2026-07-06T21:40:49Z] +     [Parameter(Mandatory = )]
[2026-07-06T21:40:49Z] +                           ~
[2026-07-06T21:40:49Z] Missing statement after '=' in named argument.
[2026-07-06T21:40:49Z] At line:5 char:27
[2026-07-06T21:40:49Z] +     [Parameter(Mandatory = )]
[2026-07-06T21:40:49Z] +                           ~
[2026-07-06T21:40:49Z] Missing closing ')' in expression.
[2026-07-06T21:40:49Z] At line:6 char:13
[2026-07-06T21:40:49Z] +     [string],
[2026-07-06T21:40:49Z] +             ~
[2026-07-06T21:40:49Z] Parameter declarations are a comma-separated list of variable names with optional initializer expressions.
[2026-07-06T21:40:49Z] At line:6 char:13
[2026-07-06T21:40:49Z] +     [string],
[2026-07-06T21:40:49Z] +             ~
[2026-07-06T21:40:49Z] Missing ')' in function parameter list.
[2026-07-06T21:40:49Z] At line:5 char:27
[2026-07-06T21:40:49Z] +     [Parameter(Mandatory = )]
[2026-07-06T21:40:49Z] +                           ~
[2026-07-06T21:40:49Z] Attribute argument must be a constant or a script block.
[2026-07-06T21:40:49Z]     + CategoryInfo          : ParserError: (:) [], ParentContainsErrorRecordException
[2026-07-06T21:40:49Z]     + FullyQualifiedErrorId : MissingExpressionInNamedArgument

ashfame · 2026-07-07T11:14:54Z

@mokagio I don't have much experience with signing apps. I asked AI to review PR 21 at WordCamp Europe and its hardening notes seemed like a good addition to be cautious. So, I asked it to split them in smaller PRs. Feel free to make this PR your own as per your discretion :)

mokagio · 2026-07-08T00:52:53Z

@ashfame no worries! And thanks for the clarification. There's always room for improvements, and these are welcome indeed.

juanmaguitar · 2026-07-10T07:24:48Z

@mokagio @ashfame what's the next step for this PR?
It's stacked on top of #27 that is already merged

iangmaia · 2026-07-15T14:31:24Z

    agents: { queue: windows }
    plugins: [$CI_TOOLKIT, $NVM_PLUGIN]
    command: |
+      $ErrorActionPreference = "Stop"


I'd suggest to move this whole command to an external script to avoid unexpected issues with the Buildkite variable interpolation (https://buildkite.com/docs/pipelines/configure/environment-variables#runtime-variable-interpolation). It also makes the code easier to read.

For an example of what might go wrong when using scripts directly in the .yml, I think $true here would need escaping so Buildkite doesn't attempt to replace it.

iangmaia · 2026-07-15T18:05:43Z

 $ErrorActionPreference = "Stop"
+Set-StrictMode -Version Latest

 & "setup_azure_trusted_signing.ps1"


So we run setup_azure_trusted_signing.ps1 from a8c-ci-toolkit-buildkite-plugin, which I think already executes signtool and loads the DLib? Then later in this script, we test the tool integrity after that.

I'd say installation, verification and execution need to be separate phases, so perhaps this should be in a8c-ci-toolkit, immediately after download and before the smoke test.

iangmaia · 2026-07-15T18:10:57Z

+    }
+
+    $actualSubject = $signature.SignerCertificate.Subject
+    if ($actualSubject.IndexOf($env:WINDOWS_EXPECTED_SIGNER_SUBJECT.Trim(), [System.StringComparison]::OrdinalIgnoreCase) -lt 0) {


I'm finding this check a bit loose (IndexOf on the subject) 🤔 Not sure if this can also change?
Maybe there's a better way, but I'm not sure how it would work (I guess we need'd to look into https://learn.microsoft.com/en-us/azure/artifact-signing/concept-certificate-management#subscriber-identity-validation-eku -- )

iangmaia · 2026-07-15T18:14:44Z

+
+foreach ($artifact in $artifacts) {
+    Write-Host "Verifying Authenticode signature for $($artifact.FullName)"
+    & $env:SIGNTOOL_PATH verify /pa /v $artifact.FullName


Looking at this, I wonder if we should consider /tw so that "a warning should be generated if the signature is not time stamped"?

Harden Azure signing hook execution

1c2b46a

ashfame requested a review from Copilot July 6, 2026 21:01

Copilot started reviewing on behalf of ashfame July 6, 2026 21:01 View session

Copilot AI reviewed Jul 6, 2026

View reviewed changes

ashfame added 2 commits July 6, 2026 21:38

Address signing hook review feedback

cefd2d9

Harden Windows signing verification in CI

6d66d8d

ashfame force-pushed the hardening/windows-buildkite-signing-verification branch from 6656710 to 6d66d8d Compare July 6, 2026 21:39

ashfame requested a review from mokagio July 6, 2026 21:42

mokagio reviewed Jul 7, 2026

View reviewed changes

Comment thread .buildkite/pipeline.yml Outdated

Remove redundant default queue specification

ba9d9f4

Base automatically changed from hardening/windows-sign-hook-timeout to trunk July 15, 2026 12:34

iangmaia reviewed Jul 15, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Harden Windows signing verification in CI#28

Harden Windows signing verification in CI#28
ashfame wants to merge 4 commits into
trunkfrom
hardening/windows-buildkite-signing-verification

ashfame commented Jul 6, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

mokagio left a comment

Uh oh!

Uh oh!

ashfame commented Jul 7, 2026

Uh oh!

mokagio commented Jul 8, 2026

Uh oh!

juanmaguitar commented Jul 10, 2026

Uh oh!

iangmaia Jul 15, 2026 •

edited

Loading

Uh oh!

iangmaia Jul 15, 2026

Uh oh!

iangmaia Jul 15, 2026

Uh oh!

iangmaia Jul 15, 2026 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Uh oh!

Conversation

ashfame commented Jul 6, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

mokagio left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ashfame commented Jul 7, 2026

Uh oh!

mokagio commented Jul 8, 2026

Uh oh!

juanmaguitar commented Jul 10, 2026

Uh oh!

iangmaia Jul 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iangmaia Jul 15, 2026

Choose a reason for hiding this comment

Uh oh!

iangmaia Jul 15, 2026

Choose a reason for hiding this comment

Uh oh!

iangmaia Jul 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

iangmaia Jul 15, 2026 •

edited

Loading

iangmaia Jul 15, 2026 •

edited

Loading