Make release signing fail closed and verify the artifact instead of its filename - #226
Merged
Merged
Conversation
…he filename Four defects meant a release could be believed signed when it was not. Both signing hooks warned and returned whenever a credential was missing. That is correct for a developer build and dangerous for a release: the build went green, the warning scrolled past in electron-builder's output, and the first contradiction arrived on a customer's machine. When a build is designated for distribution -- TRANSTRACK_RELEASE_CHANNEL=public, which release.yml now sets on both jobs, or the explicit REQUIRE_ flags -- a missing credential is now a build failure that names the variable. The release gate accepted any file matching the installer's expected name as proof of signing. verify-artifact-signature.mjs reads the artifact instead: the OS verdict on Windows, the PE Attribute Certificate Table elsewhere, with the weaker check labelled as reduced assurance rather than overstated. A catalog-only signature is rejected -- Windows calls it valid, but it lives outside the file and cannot travel with a download. Neither CI signing mode could have worked. pfx treated CSC_LINK strictly as a path, but a certificate in a CI secret is base64 bytes; it is now written to a temporary file with owner-only permissions and removed in a finally block. ssl_esigner had no ESIGNER_TOOL_PATH and no CodeSignTool on the runner. The existing signWin suite never awaited its async cases, so every assert.rejects counted as a pass without running -- the error paths had never executed. The harness is now async-aware, and two suites join it: notarize, and artifactSignature, which parses synthetic PE images on every platform and on Windows checks a genuinely signed binary and a catalog-signed one. Co-authored-by: Cursor <cursoragent@cursor.com>
…ariable The guidance to buy an EV certificate rested entirely on EV granting immediate SmartScreen reputation. Microsoft removed that behaviour; OV and EV now produce the same first-download experience, and reputation accrues per file hash either way. Following the old advice costs several hundred dollars a year for nothing except a procurement checkbox, which is a real but different reason to buy it. Two constraints that change what is even purchasable are now stated: since June 2023 every code signing key, OV included, must live in hardware, so a copyable .pfx is no longer issuable and pfx mode is for certificates already held; and since February 2026 certificates are capped at 460 days. DEPLOYMENT_PRODUCTION.md told readers to set APPLE_APP_SPECIFIC_PASSWORD, which the hook does not read. Anyone who followed it got a silent skip. The doc is corrected and the hook now names the mistake when it sees it. Validation package: TT-R146 and TT-R147 for release authenticity, SDS section 17, risk R-028, and OQ-146/147 -- the latter having the receiving site verify the installer's signature themselves before installing, which is the only check that does not depend on trusting the vendor's own build log. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Strix is installed on this repository, but we couldn't run this PR security review because this workspace's trial has ended. Add a card to resume code reviews here. |
…t tell The Windows CI job caught this on the suite's first run there: node.exe, which is genuinely signed, was reported UNSIGNED. Two causes, both of which would have failed a legitimate release. verifyAuthenticode defaulted the status to the literal string 'Unknown' when the regex found no STATUS line, so a PowerShell that produced no output at all became a verdict of "not Valid". It now returns available:false with the reason -- powershell failed to start, or exited without a verdict -- and the caller falls back to the PE certificate table, the same evidence a Linux host uses. Windows also has a real UnknownError status, meaning it could not complete trust evaluation, most often an offline revocation check. That is an absence of a conclusion, not a negative one, and is now a downgrade to 'embedded' assurance rather than a rejection. The genuinely negative statuses are unaffected: a forged certificate table answers NotSigned, which is a conclusion, and is still rejected -- verified against a fixture. Reordered the embedded-signature check ahead of the OS verdict, since whether a signature is inside the file is read from the file and does not depend on the OS being able to evaluate it. A catalog-signed binary is therefore still rejected on a machine that cannot validate chains. The two real-binary tests now adapt: they require the signature to be found either way, and assert full assurance only where trust evaluation actually works. Co-authored-by: Cursor <cursoragent@cursor.com>
…le to look The remaining Windows CI failure was the test's premise, not the verifier. It asserted that a fixture with a fake certificate table is rejected, which is only true where Get-AuthenticodeSignature answers -- and on that runner it answers nothing at all, even for node.exe. Without an OS verdict a forged table is indistinguishable from a real signature by file layout alone. So the assertion now splits along what the environment can actually establish: reject where the OS can look, and where it cannot, require that the verifier does not claim validity it did not check. The log records why no verdict was available, so the next new runner is diagnosable without a reproduction. Documented the consequence in SDS section 17 and CODE_SIGNING.md: the strongest evidence obtainable depends on where the check runs, which is why OQ-147 has the receiving site verify the installer on its own hardware -- the one execution of this check that does not depend on the vendor's build environment. Co-authored-by: Cursor <cursoragent@cursor.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Four defects in the release signing path meant a build could be believed signed when it was not. All four are fixed, and the release gate now checks the artifact rather than trusting the build configuration.
Signing failed open. Both
sign-win.cjsandnotarize.cjswarned and returned whenever a credential was missing. That is right for a developer build and dangerous for a release — the build goes green, the warning scrolls past in electron-builder's output, and the first contradiction arrives on a customer's machine. A build designated for distribution (TRANSTRACK_RELEASE_CHANNEL=public, set byrelease.ymlon both jobs, or the explicitTRANSTRACK_REQUIRE_SIGNING/TRANSTRACK_REQUIRE_NOTARIZATIONflags) now fails and names the missing variable.The gate checked the filename. "Code-signed Windows installer present" was satisfied by any file matching the expected name. New
scripts/verify-artifact-signature.mjsreads the artifact: the OS verdict viaGet-AuthenticodeSignatureon Windows, the PE Attribute Certificate Table elsewhere (the gate runs on Linux), with the weaker check labelled as reduced assurance rather than overstated. Catalog-only signatures are rejected — Windows reports them valid, but they live outside the file and cannot travel with a download.Neither CI signing mode could have worked.
pfxtreatedCSC_LINKstrictly as a filesystem path, but a certificate in a CI secret is base64 bytes; it is now materialised to a temporary file with owner-only permissions and removed in afinally.ssl_esignerhad noESIGNER_TOOL_PATHset and no CodeSignTool installed on the runner.The signer's tests never ran.
tests/signWin.test.cjsdid not await its async cases, so everyassert.rejectsincrementedPASSwithout executing. The harness is now async-aware.Also corrects the certificate guidance: the recommendation to buy EV rested on EV granting immediate SmartScreen reputation, which Microsoft removed. OV now gives the same first-download experience. The docs recommend Azure Artifact Signing (~$10/month) or OV with cloud HSM signing, and note that since June 2023 all code signing keys must live in hardware, so a copyable
.pfxis no longer issuable.DEPLOYMENT_PRODUCTION.mdsaidAPPLE_APP_SPECIFIC_PASSWORD, which the hook does not read — corrected, and the hook now diagnoses that spelling by name.Test plan
npm test— 50/50 suites, including three signing suitesnpm run lint,npm run typecheckcleannode scripts/check-compliance-docs.mjs— 83 requirements, 83 matrix rows, 73 OQ cases, 28 riskstests/artifactSignature.test.mjsparses synthetic PE32 and PE32+ images on every platform; on Windows it additionally acceptsnode.exe(genuinely signed) and rejectsnotepad.exe(catalog-signed)release/enterpriseinstaller — correctly reportsUNSIGNED [none], exit 1. The old filename check passed this same file.release.ymlparses as valid YAMLValidation package
TT-R146 (fail closed on a distribution build) and TT-R147 (verify the artifact, reject non-embedded signatures), SDS §17, risk R-028, and OQ-146/147. OQ-147 has the receiving site verify the installer's signature itself before installing, which is the only check that does not depend on trusting the vendor's build log.