Skip to content

build(win32): sign with Azure Trusted Signing via OIDC - #2004

Open
felixrieseberg wants to merge 1 commit into
mainfrom
felixr-oidc
Open

build(win32): sign with Azure Trusted Signing via OIDC#2004
felixrieseberg wants to merge 1 commit into
mainfrom
felixr-oidc

Conversation

@felixrieseberg

Copy link
Copy Markdown
Member

Replaces the DigiCert KeyLocker flow for Windows code signing with Azure Trusted Signing, authenticated through GitHub OIDC.

Changes

  • forge.config.ts: a shared @electron/windows-sign options builder for the Squirrel and MSIX makers. It writes the Trusted Signing metadata file, points signtool at the dlib, uses the ACS timestamp server and SHA-256 only. Returns undefined when no Azure variables are set, so local and CI builds stay unsigned.
  • forge.config.ts: MSIX publisher updated to the Trusted Signing certificate subject (CN=OpenJS Foundation, O=OpenJS Foundation, L=San Francisco, S=California, C=US). No MSIX has shipped in a public release, so no publisher bridging is needed.
  • release.yml: azure/login replaces the three DigiCert steps, id-token: write added to the build job, Trusted Signing account and endpoint as literals (they are resource names, not secrets), identity IDs from repository secrets.
  • @electron/windows-sign added as a devDependency for the type import.

Verification

Two runs of a temporary Windows-only workflow on this branch (since removed) exercised the full path:

Replace the DigiCert KeyLocker flow with Azure Trusted Signing. The
release workflow logs in to Azure with OIDC, installs the Trusted
Signing dlib and picks the newest Windows SDK signtool; forge.config.ts
turns those into @electron/windows-sign options shared by the Squirrel
and MSIX makers. The MSIX publisher is updated to match the Trusted
Signing certificate subject.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@felixrieseberg
felixrieseberg requested review from a team and codebytere as code owners September 4, 2026 01:11
@coveralls

Copy link
Copy Markdown

Coverage Status

coverage: 87.352%. remained the same — felixr-oidc into main

@MarshallOfSound

Copy link
Copy Markdown
Member

@claude review

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Beyond the inline finding on job-scoped id-token: write, I also checked the signtool version-selection logic (forge.config.ts:82/release.yml) and the metadata.json ExcludeCredentials handling — the SDK picker takes the newest installed kit without asserting it meets the documented 10.0.22621.755 floor, and getWindowsSignOptions()'s all-or-nothing env validation is stricter than the old single-flag check, but neither is a functional bug introduced by this diff (both fail loud/safe rather than silently misbehaving).

Extended reasoning...

The confirmed finding — id-token: write granted at the job level in .github/workflows/release.yml, applying to every OS in the build matrix rather than being scoped to the Windows OIDC leg — is a real permissions-scoping concern in a CI workflow that now mints Azure federated credentials, and will be posted inline. I independently reviewed the diff and reproduced this: the build job's permissions: block gains id-token: write alongside the existing actions: write/contents: read, with no per-step or per-OS narrowing, confirming the finding.

I also looked at the three previously ruled-out candidates to see if any deserved elevation: (1) the PowerShell signtool selection in release.yml picks the newest SDK under Windows Kits\10\bin\10.*\x64\ via Sort-Object but never checks the resolved version against the 10.0.22621.755 floor the comment cites — this could silently pick an older SDK on a runner image that has one, but GitHub-hosted windows-latest runners ship recent SDKs, and if the resolved signtool.exe genuinely lacks /dlib support, signing would fail loudly at build time rather than corrupt binaries, so this is a robustness gap rather than a security bug; (2) getWindowsSignOptions()'s throw-on-partial-config behavior is a deliberate fail-fast choice, stricter than before but not unsafe; (3) the single ExcludeCredentials array is documented Azure Identity behavior and matches how DefaultAzureCredentialOptions deserializes exclusions, so it does what the author intends.

Given a confirmed finding already exists and will be posted as an inline comment, my role here is limited to the narrow exception: adding concrete, non-duplicative context about what else was examined. I have that (the SDK-version check and the strict validation path), so I'm using decision="defer" with a short informational note rather than staying fully silent or posting a full review body, per the guidance for findings-present runs.

Comment on lines 32 to +36
permissions:
actions: write
contents: read
# Required for the OIDC login to Azure used for Windows code signing
id-token: write

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 id-token: write is granted at the job level, so it applies to every matrix runner (macOS, Linux, Windows) even though only the Windows leg needs OIDC for Azure login; before this change no runner in this workflow could mint an OIDC token. Fix: scope OIDC access to only the Windows signing work, e.g. split Windows into its own job with its own permissions block so macOS/Linux runners (which install arbitrary npm deps and run notarization scripts) don't gain the ability to request GitHub ID tokens.

Extended reasoning...

GitHub Actions permissions are job-scoped, not step-scoped, so adding id-token: write to the shared build job (matrix includes macos-latest-large, macos-latest-xlarge, ubuntu-latest, ubuntu-24.04-arm, windows-latest) exposes ACTIONS_ID_TOKEN_REQUEST_URL/TOKEN to every step on every OS, not just the new azure/login step guarded by if: startsWith(matrix.os, 'windows-'). A compromised transitive dependency pulled in during yarn install or any script run on the macOS/Linux runners could now request and exfiltrate a GitHub OIDC token (usable against any relying party trusting this repo's OIDC issuer), a capability that did not exist on any runner before this PR.

Verification: normal (security-relevant, newly exposed by this change): The diff adds id-token: write to the permissions: block of the single build job (release.yml lines 32-36). GitHub Actions permissions are job-scoped, not step-scoped, and this job runs a matrix spanning macos-latest-large, macos-latest-xlarge, ubuntu-latest, ubuntu-24.04-arm, and windows-latest (lines 15-33). Every one of those…

@dsanders11 dsanders11 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than adding @electron/windows-sign to package.json only for a type (we'll have to keep it up-to-date and in-sync with Forge), can we just derive the type directly from Forge?

type WindowsSignOptions = NonNullable<
  MakerMSIX['config']['windowsSignOptions']
>;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants