Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make Windows builds using a new ssl.com code signing certificate #3050

Merged
merged 9 commits into from
Apr 23, 2024

Conversation

philrz
Copy link
Contributor

@philrz philrz commented Apr 17, 2024

tl;dr

Our current Windows code signing certificate expires in about a week, so this gets our build up and running with a new certificate.

Details

When @nwt and I discussed getting a new certificate, we quickly learned of changes in the past year that would affect our decision. In brief, ballot CSC-17 from the CA/B Forum established new industry standards regarding the storage of private keys. This has made it such that the easiest way for us to continue signing our builds in an automated fashion would involve using a cloud-based signing service. In shopping around for the best prices, the cheapest option we found was from ssl.com that offers a 1-year OV certificate for $129 and their eSigner cloud service costs $20/mo for 20 signings with additional signings costing $1/each. Since we release a Zui Insiders build most days we could easily burn through more than 20 signings per month, but the cost doesn't seem prohibitive and if we wanted to pinch pennies we could tweak the Insiders build schedule (e.g., make it something we manually trigger when we know something worthy has been committed that justifies a new build, since often all we'll have had on a given day are advances in the Zed pointer that may not affect functionality at all.)

Having gotten it working, I've been reminded that starting over with a new certificate unfortunately lands us back in Windows Defender "unrecognized app" status. I remember this well from several years back when we first started signing the app. As always, Microsoft is intentionally opaque and doesn't disclose the details of what it takes to build up the necessary "reputation", but articles like this one and the cynical peanut gallery on HackerNews all make it pretty clear that we just need to be prepared to eat it for a couple months. We've been through this before, and other than a couple users raising mild concerns, we've survived.

Here's evidence of the Defender screens and the cert info checking out as expected for both a Zui dev build and a Zui Insiders dev build both based on this branch.

Zui Defender
Zui Cert Info
Insiders Defender
Insiders Cert Info

In terms of implementing the change, there's not a seamless integration yet with electron-builder, but @nwt and I found this comment that seemed promising and indeed my successful approach uses it pretty much verbatim. In addition to just being more code, it unfortunately also meant morphing our electron-builder config from a simple JSON file to a .js file since it requires custom signer code. That said, I was still able to maintain our approach that delivers builds for the 2x2 of (Zui, Zui Insiders) x (Dev Build, Release Build) with minimal code duplication.

I'll put some other comments inline to explain some other details of the implementation.

If this passes muster, here's a to-do list of follow-on tasks I intend to do right after this merges:

  • Get our eSigner monthly subscription set up. Right now I'm in a 30-day free grace period since I just bought the cert.
  • Update our internal "Windows Code Signing" Confluence page with details about how I bought the cert, got it set up with eSigner, and reference this PR.
  • Update the "Windows Code Signing" section of the Zui CONTRIBUTING.md.
  • Thumbs-up the comment from the guy whose comment I borrowed the code from.
  • Put something back in the Zui docs to advise users that this Windows Defender pop-up will be expected for a while. I took it out at one point when I thought we were past this.
  • Next year I'll strongly consider buying a cert with a multi-year span so we can avoid having this Windows Defender reputation-building period being an annual thing. (I've marked my calendar with a reminder and this detail.)

@philrz philrz requested review from nwt and jameskerr April 17, 2024 22:39
@philrz philrz self-assigned this Apr 17, 2024
Comment on lines 61 to 65
- name: Make values from package.json available in Actions steps
id: zui-package
uses: RadovanPelka/github-action-json@v1.0.1
with:
path: apps/zui/package.json
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added this because one of the env variables to be passed to the esigner tool is the expected filename of what would be signed, and that filename includes both the app name (Zui or Zui Insiders) and the version string, both of which are in package.json.

APPLE_ID: ${{ inputs.apple_id }}
APPLE_ID_PASSWORD: ${{ inputs.apple_id_password }}
APPLE_TEAM_ID: ${{ inputs.apple_team_id }}
CODE_SIGN_SCRIPT_PATH: ${{ github.workspace }}/esigner-codesign/dist/index.js
INPUT_COMMAND: sign
INPUT_FILE_PATH: ${{ github.workspace }}/dist/apps/zui/${{ fromJSON(steps.zui-package.outputs.productName) }} Setup ${{ fromJSON(steps.zui-package.outputs.version) }}.exe
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The fromJSON() wrappers are needed because the Action I added above to pull values from the package.json delivers strings with quotes around them, and when these remained intact the esigner tool interpreted them as parts of the filename and choked.

Comment on lines 88 to 92
- name: Check for successful signing with SignTool
if: runner.os == 'Windows'
run: |
"C:\Program Files (x86)\Microsoft SDKs\ClickOnce\SignTool\signtool.exe" verify /pa "${{ github.workspace }}/dist/apps/zui/${{ fromJSON(steps.zui-package.outputs.productName) }} Setup ${{ fromJSON(steps.zui-package.outputs.version) }}.exe"
shell: cmd
Copy link
Contributor Author

Choose a reason for hiding this comment

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

While getting the esigner tool working with electron-builder, one buggy-seeming thing I encountered is that even if the custom signing code returns false due to an error, the electron-builder apparently still behaved as if it succeeded. Here's an example run where you can see what happens: At first it looks green/successful, but if you expand the "Build Zui" step and scroll to the bottom you can see the red failure message. Bad, right?

This would have made it likely that we could one day be producing unsigned builds and not be aware. I found via web searches that this signtool verify /pa approach adequately sniffs out if an executable is, indeed, signed. I tested it out and it does what we need (example run catching an unsigned build).

// Actions taken from:
// https://github.com/electron-userland/electron-builder/issues/6158#issuecomment-1994110062
if (process.env.CODE_SIGN_SCRIPT_PATH) {
const version = zuiPackage.version;
Copy link
Contributor Author

@philrz philrz Apr 17, 2024

Choose a reason for hiding this comment

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

The code I borrowed from the GitHub issue was doing this via:

const version = execSync('node -p "require(\'./package.json\').version"').toString().trim();

I don't know why he didn't opt to just require the package.json and access the value directly like I'm doing there. My approach feels cleaner, so I'm going with it. But I figured I'd point it out in case I'm missing something subtle.

@jameskerr
Copy link
Member

I learned here that we can keep electron-builder.json and provide a path to a file responsible for signing.

https://www.electron.build/tutorials/code-signing-windows-apps-on-unix#integrate-signing-with-electron-builder

I might need a zoom to talk more about how this all works.

@philrz
Copy link
Contributor Author

philrz commented Apr 23, 2024

Thanks for the improvements, @jameskerr. I had to push a couple more small commits to make things run successfully in Actions but I ultimately did successfully make both a Zui Dev build and a Zui Insiders Dev Build both based on c048ce8 from this branch. I manually checked the validity of the certificates by looking at the Properties of the .exe installers on windows and installed/ran them both successfully. I'm certainly 👍 on your improvements so if you're ok clicking Approve based on what else I've put up here I think we're good to merge.

@philrz philrz merged commit 2d23f20 into main Apr 23, 2024
12 of 13 checks passed
@philrz philrz deleted the ssl-com-esigner branch April 23, 2024 19:23
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.

None yet

2 participants