Skip to content

ci(app): Azure trusted signing for Windows#18450

Merged
y3rsh merged 2 commits intoedgefrom
azure-trusted-signing-for-electron-20250529
May 29, 2025
Merged

ci(app): Azure trusted signing for Windows#18450
y3rsh merged 2 commits intoedgefrom
azure-trusted-signing-for-electron-20250529

Conversation

@y3rsh
Copy link
Copy Markdown
Member

@y3rsh y3rsh commented May 29, 2025

Overview

Replace DigiCert with Azure Trusted Signing.
Helpful Blog
Helpful GitHub Issue
Electron Builder Docs

  • Added the 3 secrets
  • AZURE_TENANT_ID: ${{secrets.AZURE_TENANT_ID}}
  • AZURE_CLIENT_ID: ${{secrets.AZURE_CLIENT_ID}}
  • AZURE_CLIENT_SECRET: ${{secrets.AZURE_CLIENT_SECRET}}

After ~ July 12 we can delete all the now unused secrets for DigiCert once everything expires. Don't do it until then just in case.

Test Plan and Hands on Testing

Review requests

Things all mapped correctly?

Risk assessment

  • Yes, using beta Electron builder feature and new vendor for the process.

@y3rsh y3rsh self-assigned this May 29, 2025
@y3rsh y3rsh requested review from a team as code owners May 29, 2025 14:32
@y3rsh y3rsh requested review from ncdiehl11 and removed request for a team May 29, 2025 14:32
@codecov
Copy link
Copy Markdown

codecov Bot commented May 29, 2025

Codecov Report

Attention: Patch coverage is 0% with 5 lines in your changes missing coverage. Please review.

Project coverage is 26.47%. Comparing base (df970fe) to head (aa37d79).
Report is 7 commits behind head on edge.

Files with missing lines Patch % Lines
app-shell/electron-builder.config.js 0.00% 5 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             edge   #18450      +/-   ##
==========================================
+ Coverage   25.16%   26.47%   +1.31%     
==========================================
  Files        3254     3254              
  Lines      277294   277336      +42     
  Branches    32247    32247              
==========================================
+ Hits        69772    73434    +3662     
+ Misses     207499   203873    -3626     
- Partials       23       29       +6     
Flag Coverage Δ
app 3.06% <0.00%> (+2.06%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
app-shell/electron-builder.config.js 0.00% <0.00%> (ø)

... and 79 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@y3rsh y3rsh requested review from mjhuff, neo-jesse and sfoster1 May 29, 2025 14:36
Copy link
Copy Markdown
Member

@sfoster1 sfoster1 left a comment

Choose a reason for hiding this comment

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

Looks great if it works! Make a branch that ends with -app-build-both-as-release to build and sign both internal and release variants

Copy link
Copy Markdown
Contributor

@mjhuff mjhuff left a comment

Choose a reason for hiding this comment

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

🎉

@y3rsh y3rsh merged commit b5cfdf4 into edge May 29, 2025
51 of 55 checks passed
@y3rsh y3rsh deleted the azure-trusted-signing-for-electron-20250529 branch May 29, 2025 16:32
ddcc4 pushed a commit that referenced this pull request May 29, 2025
sfoster1 added a commit that referenced this pull request Jun 30, 2025
sfoster1 added a commit that referenced this pull request Jul 1, 2025
This reverts #18450 and #18479 to revert to digicert signing for our
windows builds.

The digicert certificate has the Common Name "Opentrons Labworks Inc."
and the ATS cert has the common name "OPENTRONS LABWORKS INC.". These
were both determined automatically by the CA from our identity
submissions, as is apparently required in the code signing cert baseline
requirements. Why are they different? A mystery for the ages.

In either case, electron-updater requires that _if_ you specify a
`publisherName` in your `app-update.yml` (which we do specifically on
windows, since it is generated from our electron-builder config and on
windows we set it because nsis packager wants it for doing signing in
the first place) _then_ the autoupdate package that will be installed
must have a CSC CN exactly matching an entry in `publisherName` or the
update will fail. Therefore updates in between <=8.4.1 and >=8.5.0 would
fail if we switched to ATS.

Instead, we'll switch back to digicert for now; we'll build the new CN
into our publisher names; and then whenever we're confident enough
people are on >=8.5.0 and therefore have the new publisher names, we'll
switch over again (we can't switch immediately because we don't do
incremental updates, just full overwrites, so the intermediate update
state would go away).

This is upsetting.

## Testing
- [x] the signing has to work again, which is never guaranteed given the
shonky state of dco integration
- [x] we should make sure we can update from 8.4.1 to this by making
sure the CN of the digital signature on the build from this pr is
exactly `Opentrons Labworks Inc.` (and updating to it in the resulting
alpha)
- [x] we should make sure we can update from this to something signed
with the new cert (by mucking around with the latest-alpha or something?
or just checking that the app-update.yml in the app's install directory
has both names)

Supercedes #18785 for build branch name reasons.
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.

3 participants