From 1c2b46ab531db773d576c713806b90f40447e587 Mon Sep 17 00:00:00 2001 From: ashfame Date: Mon, 6 Jul 2026 20:59:15 +0000 Subject: [PATCH 1/4] Harden Azure signing hook execution --- scripts/azure-sign.cjs | 37 ++++++++++++++++++++++++++++++++----- test/azure-sign.test.cjs | 23 ++++++++++++++++++++++- 2 files changed, 54 insertions(+), 6 deletions(-) diff --git a/scripts/azure-sign.cjs b/scripts/azure-sign.cjs index 984e164..bf9d884 100644 --- a/scripts/azure-sign.cjs +++ b/scripts/azure-sign.cjs @@ -11,6 +11,7 @@ const { spawnSync } = require('node:child_process'); const REQUIRED_ENV_VARS = ['SIGNTOOL_PATH', 'AZURE_CODE_SIGNING_DLIB', 'AZURE_METADATA_JSON']; +const DEFAULT_SIGNTOOL_TIMEOUT_MS = 10 * 60 * 1000; function nonBlank(value) { return typeof value === 'string' && value.trim() !== '' ? value.trim() : undefined; @@ -20,17 +21,28 @@ function shouldSign(env) { return REQUIRED_ENV_VARS.every((name) => nonBlank(env[name]) !== undefined); } +function signtoolTimeoutMs(env) { + const rawTimeout = nonBlank(env.SIGNTOOL_TIMEOUT); + if (rawTimeout === undefined || !/^\d+$/.test(rawTimeout)) { + return DEFAULT_SIGNTOOL_TIMEOUT_MS; + } + + const timeout = Number.parseInt(rawTimeout, 10); + return timeout > 0 ? timeout : DEFAULT_SIGNTOOL_TIMEOUT_MS; +} + +function debugEnabled(env) { + return ['1', 'true', 'yes'].includes((nonBlank(env.AZURE_SIGN_DEBUG) || '').toLowerCase()); +} + function buildSigntoolArgs(file, env) { const fileDigest = nonBlank(env.AZURE_FILE_DIGEST) || 'SHA256'; const timestampServer = nonBlank(env.AZURE_TIMESTAMP_SERVER) || 'http://timestamp.acs.microsoft.com'; const timestampDigest = nonBlank(env.AZURE_TIMESTAMP_DIGEST) || 'SHA256'; - // `/debug` surfaces Azure auth/quota/network diagnostics on failure instead of a generic - // SignTool error. - return [ + const args = [ 'sign', '/v', - '/debug', '/fd', fileDigest, '/tr', timestampServer, '/td', timestampDigest, @@ -38,6 +50,13 @@ function buildSigntoolArgs(file, env) { '/dmdf', nonBlank(env.AZURE_METADATA_JSON), file, ]; + + // `/debug` can expose Azure account/profile diagnostics in CI logs, so keep it opt-in. + if (debugEnabled(env)) { + args.splice(2, 0, '/debug'); + } + + return args; } module.exports = async function sign(configuration) { @@ -52,9 +71,16 @@ module.exports = async function sign(configuration) { } console.log(`[azure-sign] Signing ${file} with Azure Trusted Signing`); - const result = spawnSync(nonBlank(env.SIGNTOOL_PATH), buildSigntoolArgs(file, env), { stdio: 'inherit' }); + const timeout = signtoolTimeoutMs(env); + const result = spawnSync(nonBlank(env.SIGNTOOL_PATH), buildSigntoolArgs(file, env), { + stdio: 'inherit', + timeout, + }); if (result.error) { + if (result.error.code === 'ETIMEDOUT') { + throw new Error(`[azure-sign] signtool timed out after ${timeout}ms signing ${file}`); + } throw result.error; } if (result.signal) { @@ -67,3 +93,4 @@ module.exports = async function sign(configuration) { module.exports.shouldSign = shouldSign; module.exports.buildSigntoolArgs = buildSigntoolArgs; +module.exports.signtoolTimeoutMs = signtoolTimeoutMs; diff --git a/test/azure-sign.test.cjs b/test/azure-sign.test.cjs index 9058d26..6630ceb 100644 --- a/test/azure-sign.test.cjs +++ b/test/azure-sign.test.cjs @@ -1,7 +1,7 @@ const test = require('node:test'); const assert = require('node:assert/strict'); -const { shouldSign, buildSigntoolArgs } = require('../scripts/azure-sign.cjs'); +const { shouldSign, buildSigntoolArgs, signtoolTimeoutMs } = require('../scripts/azure-sign.cjs'); const FULL_ENV = { SIGNTOOL_PATH: 'C:/sdk/x64/signtool.exe', @@ -41,6 +41,13 @@ test('buildSigntoolArgs signs SHA256-only with dlib, metadata, timestamp, and ta assert.equal(valueAfter(args, '/dmdf'), FULL_ENV.AZURE_METADATA_JSON); assert.equal(args.at(-1), 'app.exe'); assert.ok(!args.includes('SHA1')); + assert.ok(!args.includes('/debug')); +}); + +test('buildSigntoolArgs includes debug diagnostics only when enabled', () => { + const args = buildSigntoolArgs('app.exe', { ...FULL_ENV, AZURE_SIGN_DEBUG: 'true' }); + + assert.ok(args.includes('/debug')); }); test('buildSigntoolArgs trims surrounding whitespace from dlib and metadata paths', () => { @@ -79,3 +86,17 @@ test('buildSigntoolArgs honors the toolkit-exported digest/timestamp overrides', assert.equal(valueAfter(args, '/tr'), 'http://ts.example/test'); assert.equal(valueAfter(args, '/td'), 'SHA512'); }); + +test('signtoolTimeoutMs defaults to ten minutes', () => { + assert.equal(signtoolTimeoutMs({}), 10 * 60 * 1000); +}); + +test('signtoolTimeoutMs honors positive SIGNTOOL_TIMEOUT values', () => { + assert.equal(signtoolTimeoutMs({ SIGNTOOL_TIMEOUT: '30000' }), 30000); +}); + +test('signtoolTimeoutMs ignores blank, invalid, and zero SIGNTOOL_TIMEOUT values', () => { + assert.equal(signtoolTimeoutMs({ SIGNTOOL_TIMEOUT: ' ' }), 10 * 60 * 1000); + assert.equal(signtoolTimeoutMs({ SIGNTOOL_TIMEOUT: 'soon' }), 10 * 60 * 1000); + assert.equal(signtoolTimeoutMs({ SIGNTOOL_TIMEOUT: '0' }), 10 * 60 * 1000); +}); From cefd2d9dc5f336897d728a13c4b11b48a7ff1f84 Mon Sep 17 00:00:00 2001 From: ashfame Date: Mon, 6 Jul 2026 21:38:35 +0000 Subject: [PATCH 2/4] Address signing hook review feedback --- scripts/azure-sign.cjs | 13 +++++++------ test/azure-sign.test.cjs | 6 ++++++ 2 files changed, 13 insertions(+), 6 deletions(-) diff --git a/scripts/azure-sign.cjs b/scripts/azure-sign.cjs index bf9d884..2a3061b 100644 --- a/scripts/azure-sign.cjs +++ b/scripts/azure-sign.cjs @@ -12,6 +12,7 @@ const { spawnSync } = require('node:child_process'); const REQUIRED_ENV_VARS = ['SIGNTOOL_PATH', 'AZURE_CODE_SIGNING_DLIB', 'AZURE_METADATA_JSON']; const DEFAULT_SIGNTOOL_TIMEOUT_MS = 10 * 60 * 1000; +const MAX_SIGNTOOL_TIMEOUT_MS = 60 * 60 * 1000; function nonBlank(value) { return typeof value === 'string' && value.trim() !== '' ? value.trim() : undefined; @@ -28,7 +29,11 @@ function signtoolTimeoutMs(env) { } const timeout = Number.parseInt(rawTimeout, 10); - return timeout > 0 ? timeout : DEFAULT_SIGNTOOL_TIMEOUT_MS; + if (!Number.isSafeInteger(timeout) || timeout <= 0 || timeout > MAX_SIGNTOOL_TIMEOUT_MS) { + return DEFAULT_SIGNTOOL_TIMEOUT_MS; + } + + return timeout; } function debugEnabled(env) { @@ -43,6 +48,7 @@ function buildSigntoolArgs(file, env) { const args = [ 'sign', '/v', + ...(debugEnabled(env) ? ['/debug'] : []), '/fd', fileDigest, '/tr', timestampServer, '/td', timestampDigest, @@ -51,11 +57,6 @@ function buildSigntoolArgs(file, env) { file, ]; - // `/debug` can expose Azure account/profile diagnostics in CI logs, so keep it opt-in. - if (debugEnabled(env)) { - args.splice(2, 0, '/debug'); - } - return args; } diff --git a/test/azure-sign.test.cjs b/test/azure-sign.test.cjs index 6630ceb..f4aacfa 100644 --- a/test/azure-sign.test.cjs +++ b/test/azure-sign.test.cjs @@ -48,6 +48,7 @@ test('buildSigntoolArgs includes debug diagnostics only when enabled', () => { const args = buildSigntoolArgs('app.exe', { ...FULL_ENV, AZURE_SIGN_DEBUG: 'true' }); assert.ok(args.includes('/debug')); + assert.deepEqual(args.slice(0, 6), ['sign', '/v', '/debug', '/fd', 'SHA256', '/tr']); }); test('buildSigntoolArgs trims surrounding whitespace from dlib and metadata paths', () => { @@ -100,3 +101,8 @@ test('signtoolTimeoutMs ignores blank, invalid, and zero SIGNTOOL_TIMEOUT values assert.equal(signtoolTimeoutMs({ SIGNTOOL_TIMEOUT: 'soon' }), 10 * 60 * 1000); assert.equal(signtoolTimeoutMs({ SIGNTOOL_TIMEOUT: '0' }), 10 * 60 * 1000); }); + +test('signtoolTimeoutMs ignores unsafe and over-limit SIGNTOOL_TIMEOUT values', () => { + assert.equal(signtoolTimeoutMs({ SIGNTOOL_TIMEOUT: '3600001' }), 10 * 60 * 1000); + assert.equal(signtoolTimeoutMs({ SIGNTOOL_TIMEOUT: '999999999999999999999999999999999999999' }), 10 * 60 * 1000); +}); From 6d66d8d240df2265dbbabd964bac046a33d03f79 Mon Sep 17 00:00:00 2001 From: ashfame Date: Mon, 6 Jul 2026 20:59:59 +0000 Subject: [PATCH 3/4] Harden Windows signing verification in CI --- .../commands/setup_windows_code_signing.ps1 | 62 +++++++++++++++++++ .../commands/verify_windows_signature.ps1 | 51 +++++++++++++++ .buildkite/pipeline.yml | 40 +++++++++--- 3 files changed, 146 insertions(+), 7 deletions(-) create mode 100755 .buildkite/commands/verify_windows_signature.ps1 diff --git a/.buildkite/commands/setup_windows_code_signing.ps1 b/.buildkite/commands/setup_windows_code_signing.ps1 index 4fa8af1..9f9282a 100755 --- a/.buildkite/commands/setup_windows_code_signing.ps1 +++ b/.buildkite/commands/setup_windows_code_signing.ps1 @@ -7,6 +7,68 @@ # Windows AMI as of a8c-ci-toolkit 6.0.0, so there is no separate host-preparation step. $ErrorActionPreference = "Stop" +Set-StrictMode -Version Latest & "setup_azure_trusted_signing.ps1" if ($LASTEXITCODE -ne 0) { exit $LASTEXITCODE } + +function Assert-SigningToolIntegrity { + param ( + [Parameter(Mandatory = $true)] + [string]$Path, + [Parameter(Mandatory = $true)] + [string]$Description, + [Parameter(Mandatory = $true)] + [string]$ExpectedSha256EnvVar + ) + + if ([string]::IsNullOrWhiteSpace($Path)) { + Write-Host "[!] $Description path is not set." + exit 1 + } + + if (-not (Test-Path -LiteralPath $Path)) { + Write-Host "[!] $Description was not found at $Path." + exit 1 + } + + $expectedSha256 = [Environment]::GetEnvironmentVariable($ExpectedSha256EnvVar) + if (-not [string]::IsNullOrWhiteSpace($expectedSha256)) { + $actualSha256 = (Get-FileHash -LiteralPath $Path -Algorithm SHA256).Hash + if ($actualSha256 -ne $expectedSha256.Trim().ToUpperInvariant()) { + Write-Host "[!] $Description SHA256 mismatch." + Write-Host "Expected: $($expectedSha256.Trim().ToUpperInvariant())" + Write-Host "Actual: $actualSha256" + exit 1 + } + + Write-Host "$Description SHA256 matched $ExpectedSha256EnvVar." + return + } + + $signature = Get-AuthenticodeSignature -FilePath $Path + if ($signature.Status -ne "Valid" -or $null -eq $signature.SignerCertificate) { + Write-Host "[!] $Description does not have a valid Authenticode signature." + Write-Host "Status: $($signature.Status)" + exit 1 + } + + $subject = $signature.SignerCertificate.Subject + if ($subject.IndexOf("Microsoft", [System.StringComparison]::OrdinalIgnoreCase) -lt 0) { + Write-Host "[!] $Description is not signed by an expected Microsoft certificate." + Write-Host "Actual signer subject: $subject" + exit 1 + } + + Write-Host "$Description Authenticode signature is valid: $subject" +} + +Assert-SigningToolIntegrity ` + -Path $env:SIGNTOOL_PATH ` + -Description "signtool.exe" ` + -ExpectedSha256EnvVar "WINDOWS_SIGNTOOL_SHA256" + +Assert-SigningToolIntegrity ` + -Path $env:AZURE_CODE_SIGNING_DLIB ` + -Description "Azure Trusted Signing DLib" ` + -ExpectedSha256EnvVar "AZURE_CODE_SIGNING_DLIB_SHA256" diff --git a/.buildkite/commands/verify_windows_signature.ps1 b/.buildkite/commands/verify_windows_signature.ps1 new file mode 100755 index 0000000..c5e8cf2 --- /dev/null +++ b/.buildkite/commands/verify_windows_signature.ps1 @@ -0,0 +1,51 @@ +$ErrorActionPreference = "Stop" +Set-StrictMode -Version Latest + +$artifacts = @(Get-ChildItem -Path "dist\*.exe" -File -ErrorAction SilentlyContinue) +if ($artifacts.Count -eq 0) { + Write-Host "[!] No Windows .exe artifacts found in dist." + exit 1 +} + +if ([string]::IsNullOrWhiteSpace($env:SIGNTOOL_PATH)) { + Write-Host "[!] SIGNTOOL_PATH is not set." + exit 1 +} + +if ([string]::IsNullOrWhiteSpace($env:WINDOWS_EXPECTED_SIGNER_SUBJECT)) { + Write-Host "[!] WINDOWS_EXPECTED_SIGNER_SUBJECT must be set to verify the artifact signer identity." + exit 1 +} + +foreach ($artifact in $artifacts) { + Write-Host "Verifying Authenticode signature for $($artifact.FullName)" + & $env:SIGNTOOL_PATH verify /pa /v $artifact.FullName + if ($LASTEXITCODE -ne 0) { exit $LASTEXITCODE } + + $signature = Get-AuthenticodeSignature -FilePath $artifact.FullName + if ($signature.Status -ne "Valid" -or $null -eq $signature.SignerCertificate) { + Write-Host "[!] $($artifact.Name) does not have a valid Authenticode signature." + Write-Host "Status: $($signature.Status)" + exit 1 + } + + $actualSubject = $signature.SignerCertificate.Subject + if ($actualSubject.IndexOf($env:WINDOWS_EXPECTED_SIGNER_SUBJECT.Trim(), [System.StringComparison]::OrdinalIgnoreCase) -lt 0) { + Write-Host "[!] $($artifact.Name) was not signed by the expected subject." + Write-Host "Expected subject to contain: $($env:WINDOWS_EXPECTED_SIGNER_SUBJECT.Trim())" + Write-Host "Actual signer subject: $actualSubject" + exit 1 + } + + if (-not [string]::IsNullOrWhiteSpace($env:WINDOWS_EXPECTED_SIGNER_ISSUER)) { + $actualIssuer = $signature.SignerCertificate.Issuer + if ($actualIssuer.IndexOf($env:WINDOWS_EXPECTED_SIGNER_ISSUER.Trim(), [System.StringComparison]::OrdinalIgnoreCase) -lt 0) { + Write-Host "[!] $($artifact.Name) was not signed by the expected issuer." + Write-Host "Expected issuer to contain: $($env:WINDOWS_EXPECTED_SIGNER_ISSUER.Trim())" + Write-Host "Actual signer issuer: $actualIssuer" + exit 1 + } + } + + Write-Host "$($artifact.Name) signer identity verified." +} diff --git a/.buildkite/pipeline.yml b/.buildkite/pipeline.yml index 2192e8e..6114960 100644 --- a/.buildkite/pipeline.yml +++ b/.buildkite/pipeline.yml @@ -9,28 +9,54 @@ steps: agents: { queue: windows } plugins: [$CI_TOOLKIT, $NVM_PLUGIN] command: | + $ErrorActionPreference = "Stop" + + function Invoke-NativeCommand { + param ( + [Parameter(Mandatory = $true)] + [string]$FilePath, + [Parameter(ValueFromRemainingArguments = $true)] + [string[]]$ArgumentList + ) + + & $FilePath @ArgumentList + if ($LASTEXITCODE -ne 0) { exit $LASTEXITCODE } + } + echo "~~~ Setup code signing" .buildkite/commands/setup_windows_code_signing.ps1 echo "~~~ Install dependencies" - bash .buildkite/commands/install_node_dependencies.sh + Invoke-NativeCommand bash .buildkite/commands/install_node_dependencies.sh echo "~~~ Build renderer" - npm run build:once + Invoke-NativeCommand npm run build:once echo "~~~ Build Windows artifact" - npm run dist:win + Invoke-NativeCommand npm run dist:win echo "~~~ Verify Azure Trusted Signing signature" - Get-ChildItem dist\*.exe | ForEach-Object { - & $env:SIGNTOOL_PATH verify /pa /v $_.FullName - if ($LASTEXITCODE -ne 0) { exit $LASTEXITCODE } - } + .buildkite/commands/verify_windows_signature.ps1 artifact_paths: - dist\*.exe notify: - github_commit_status: { context: "Windows Build" } + ######################################################## + # Tests + ######################################################## + - label: ":node: Tests" + agents: { queue: default } + plugins: [$CI_TOOLKIT, $NVM_PLUGIN] + command: | + echo "~~~ Install dependencies" + .buildkite/commands/install_node_dependencies.sh + + echo "~~~ Run tests" + npm test + notify: + - github_commit_status: { context: "Tests" } + ######################################################## # macOS build ######################################################## From ba9d9f4b19bf53e07108b1e74356894e8c141527 Mon Sep 17 00:00:00 2001 From: Gio Lodi Date: Wed, 8 Jul 2026 10:52:11 +1000 Subject: [PATCH 4/4] Remove redundant default queue specification --- .buildkite/pipeline.yml | 1 - 1 file changed, 1 deletion(-) diff --git a/.buildkite/pipeline.yml b/.buildkite/pipeline.yml index 6114960..1d51d77 100644 --- a/.buildkite/pipeline.yml +++ b/.buildkite/pipeline.yml @@ -46,7 +46,6 @@ steps: # Tests ######################################################## - label: ":node: Tests" - agents: { queue: default } plugins: [$CI_TOOLKIT, $NVM_PLUGIN] command: | echo "~~~ Install dependencies"