Skip to content

fix(app): revert to digicert signing#18785

Closed
sfoster1 wants to merge 3 commits intochore_release-8.5.0from
rqa-4324-worst-day-of-my-life-app-build-both
Closed

fix(app): revert to digicert signing#18785
sfoster1 wants to merge 3 commits intochore_release-8.5.0from
rqa-4324-worst-day-of-my-life-app-build-both

Conversation

@sfoster1
Copy link
Copy Markdown
Member

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

  • the signing has to work again, which is never guaranteed given the shonky state of dco integration
  • 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)
  • 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)

sfoster1 added 3 commits June 30, 2025 17:23
This will allow users to update to the new app whenever we switch to
azure trusted signing.
@sfoster1 sfoster1 requested a review from y3rsh June 30, 2025 21:35
@sfoster1 sfoster1 requested review from a team as code owners June 30, 2025 21:35
@sfoster1 sfoster1 requested review from mjhuff and removed request for a team June 30, 2025 21:35
Copy link
Copy Markdown
Member

@y3rsh y3rsh left a comment

Choose a reason for hiding this comment

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

😿

@codecov
Copy link
Copy Markdown

codecov Bot commented Jul 1, 2025

Codecov Report

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

Project coverage is 24.43%. Comparing base (d80588d) to head (375a9be).
Report is 1 commits behind head on chore_release-8.5.0.

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

Impacted file tree graph

@@                   Coverage Diff                    @@
##           chore_release-8.5.0   #18785       +/-   ##
========================================================
- Coverage                54.64%   24.43%   -30.22%     
========================================================
  Files                     3258     3258               
  Lines                   281788   281757       -31     
  Branches                 29494    29463       -31     
========================================================
- Hits                    153995    68843    -85152     
- Misses                  127603   212887    +85284     
+ Partials                   190       27      -163     
Flag Coverage Δ
app 3.19% <0.00%> (-44.11%) ⬇️

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%> (ø)
app-shell/scripts/windows-custom-sign.js 0.00% <0.00%> (ø)

... and 1619 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.

@sfoster1
Copy link
Copy Markdown
Member Author

sfoster1 commented Jul 1, 2025

Superceded by #18792 which has the right branch name to actually sign builds.

@sfoster1 sfoster1 closed this Jul 1, 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.

2 participants