Harden Windows signing verification in CI by ashfame · Pull Request #28 · WordPress/contributor-toolkit

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"?

Three conflicts, all resolved in trunk's favour: - `.buildkite/pipeline.yml`: this branch ran `npm run build:once` as its own Buildkite step before `npm run dist:win`. Trunk's `dist:win` now chains `build:once` itself (#122), so the explicit step is redundant. The hardening this branch adds — `Invoke-NativeCommand` around the native calls, and the extracted `verify_windows_signature.ps1` — is kept. - `scripts/azure-sign.cjs`: trunk landed the same `/debug` passthrough with an explanatory comment; kept the comment. - `test/azure-sign.test.cjs`: trunk moved the suite to `tests/unit/`, so the `require` of `scripts/azure-sign.cjs` gains a path segment. `npm test` (1046 passing) and `npm run lint` are green on the merge result. --- Generated with the help of Claude Code, https://claude.com/claude-code Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Added here on 2026-07-06, when `npm test` ran in no CI at all. #68 landed three weeks later and now runs the unit suite in GitHub Actions on macOS and Windows, on both the system Node and the one Electron bundles — so this step is a strict subset of that, on a self-hosted agent that holds the signing credentials. The suite needs no secrets, which is what makes Actions the right place for it: #65 sets the split as everything secretless on Actions, Buildkite only for the signed artifacts, and lists "Buildkite stays untouched and keeps producing the signed artifacts" as a constraint. --- Generated with the help of Claude Code, https://claude.com/claude-code Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

mokagio · 2026-08-21T02:56:31Z

@ashfame you'll notice I merged trunk in this PR. It was to carry over some structural and security updates.

I haven't had the time to look at how it behaves yet, not to address @iangmaia or Copilot's comment.

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

mokagio mentioned this pull request Aug 21, 2026

Validate the trunk merge for #28 (do not merge) #394

Closed

Navigation Menu

Uh oh!

Harden Windows signing verification in CI - #28

Harden Windows signing verification in CI#28
ashfame wants to merge 6 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!

mokagio commented Aug 21, 2026

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!

mokagio commented Aug 21, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

iangmaia Jul 15, 2026 •

edited

Loading

iangmaia Jul 15, 2026 •

edited

Loading