-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Support for Azure Trusted Signing in Electron Builder #8276
Comments
I guess a lot of those powershell commands could be copy-pasted/translated into electron-builder's signing workflow, but if any updates happen to that github action, they won't be propagated into electron-builder without someone opening an issue here. Would that be a problem? |
We're also interested in this. There is a nice guide for implementation of trusted EV code signing using Azure here: https://melatonin.dev/blog/code-signing-on-windows-with-azure-trusted-signing/ Basically, we'd need to call the action multiple times:
Currently, it seems that steps 1 and 2 are done in one action and therefore it's not possible to execute code signing after each of the steps using the GitHub action, right? |
From the github action code it looks like a powershell script that installs the TrustedSigning module and then invokes it with appropriate params is all that is required: Install-Module -Name TrustedSigning -RequiredVersion 0.4.1 -Force -Repository PSGallery
Invoke-TrustedSigning @params This should be possible to add to electron-builder with some extra env vars? |
I quickly put together a basic copy of the action code for an Basically, we skip the signing stage for the regular signing process. Instead, we leverage the Would you be willing to do some investigating from there? Not sure if the params need to be passed in as a var or as a literal string. I don't have any azure account to test this myself
|
Can confirm that this approach works! Here is what I ended up using:
...
afterSign: ./scripts/after-sign.js
win:
sign: ./scripts/nop.js
const { spawnSync } = require('node:child_process');
exports.default = async function sign(context) {
spawnSync(
'powershell.exe',
['Install-Module', '-Name', 'TrustedSigning', '-RequiredVersion', '0.4.1', '-Force', '-Repository', 'PSGallery'],
{ shell: true, stdio: 'inherit' },
);
const params = {
Endpoint: 'https://eus.codesigning.azure.net/',
CodeSigningAccountName: '<code signing account name>',
CertificateProfileName: '<certificate profile name>',
FilesFolder: context.appOutDir,
FilesFolderFilter: 'exe,dll',
FileDigest: 'SHA256',
TimestampRfc3161: 'http://timestamp.acs.microsoft.com',
TimestampDigest: 'SHA256',
};
spawnSync('powershell.exe', ['Invoke-TrustedSigning', params], { shell: true, stdio: 'inherit' });
};
exports.default = async function nop() {}; |
@MikeJerred Wouldn't your proposed solution result in only the packaged executables being signed, not the installer (e.g. NSIS executable file) that results from the packaging process? It seems that the Another problem I've spotted: When testing your script, I received the following output const paramsString = Object.keys(params).map(key => ` -${key} ${params[key]}`).join('');
spawnSync('powershell.exe', ['Invoke-TrustedSigning', paramsString], { shell: true, stdio: 'inherit' }); |
I just received confirmation from MS that I have approval for the trusted signing cert. Based on some of the above hacks, I may have to skip Windows signing and use a signtool manually since installers as well as app needs to be signed. |
Signing the installer after the electron-builder packing process during and extra build step results in wrong sha512 hashes in the resulting update YAML files. |
I've managed to get this signed with my own script during the build...all executables checkout out nicely. Gone are the days of EV certs. |
I'm looking into implementing this in electron-builder, but won't have a way to test (as I don't have any Azure account). So if anyone is willing, I'd be happy to supply a patch-package patch for testing out my implementation. What are the required params for
I also see this example configuration here: https://learn.microsoft.com/en-us/azure/trusted-signing/how-to-signing-integrations
Reason I ask is to see if there are any default values I can apply or using enums (for things like |
I am happy to help with testing. Those params (plus the azure auth env vars) were enough for the signing to complete without errors when I tried it. |
Okay, nvm, the patch is too large. My best recommendation is cloning electron-builder, pulling this PR #8458 via From there, the configuration is within electron-builder/packages/app-builder-lib/src/options/winOptions.ts Lines 178 to 202 in 0d24b78
|
Not sure if my local dev setup is correct because I get an error when doing this:
|
You'll either need to start with the base v25.0.5 installed in your package.json (as that includes most recent dependencies to be resolved during
|
That has fixed that issue, however it still doesn't work:
|
Looks like You can copy paste it manually in your node modules path or take the hard-copy approach (instead of using
|
@MikeJerred I'm thinking of releasing the refactored signing code as part of 25.0.7 with logging that azure signing is in Once released, I would like additional volunteers to test it though with |
You could also possibly release it on the beta release channel, if the changes are too impactful? Anyway, also switching here to Azure Trusted Signing, as our previous certificate was expired. |
Excellent! Thank you Previous logic is all in place for using It'll be default released to |
Alrighty. Beta signing implementation has been released in I'm expecting bug reports, so I also request patience as I get this implementation fully functional. 🙃 Also, not sure if the cmd line debug logs will need any info redacted before posting them here since it's a verbatim log of the powershell From my local testing, I got this working up until the point it does Logs below with
Implementation details: https://github.com/electron-userland/electron-builder/blob/master/packages/app-builder-lib/src/codeSign/windowsSignAzureManager.ts Configuration details: electron-builder/packages/app-builder-lib/src/options/winOptions.ts Lines 88 to 91 in b3ce7f7
electron-builder/packages/app-builder-lib/src/options/winOptions.ts Lines 190 to 210 in b3ce7f7
|
I installed 25.1.0 but doing
The app-builder-lib in node_modules is v25.0.5, and it doesn't have a |
Thanks for checking. Hmmm, it sounds like it desynced the release versioning during the CI/CD. It's been acting finicky lately. Can you try force installing I'll look into the dependency resolution issue |
I added
|
Kk. I've redeployed the monorepo to resync all the workspace versions. Please try |
OK this version installs properly. It should also be noted that the docs are stating to use This is my
I also have set the 3 env vars |
@MikeJerred Can you please upload/send the logs for the azure signing steps?
Please make sure to redact any sensitive info from the logs if present. Also am happy to discuss further via discord (@onegoldfish) to streamline debugging/implementing this feature. |
This is the log, I redacted some long bits that I don't think are relevant:
|
Also not working here. At least not signing. I see that this line is not returning valid data:
As when I set forceCodeSigning to Effective config being print:
Also, is it normal that I had to had to run it in a elevated cmd prompt? I got this error message: |
Great investigate work! Thank you :) FWIW, I'm honestly shocked that this was not caught in the code signing unit tests. I'm also struggling to reproduce this locally in my test project with config
Logs of successful build using signtool:
I'll see if I can mock a way to do the azure signing method without calling Invoke-TrustedSigning since I can't get an azure test account for free AFAIK, but at least it could test the initial logic in that electron-builder flow? I'll see what I can do |
Okay, I did a bit more refactoring and moved some of the signtool logic that was still in winPackager into the signtool manager class (#8524)
[EDIT], patch didn't work, removing from comment to reduce verbosity of this thread/GH issue |
Thanks for your changes @mmaietta , will try them out tonight.
But, there was probably no problem with normal signing. The problem is that electron-builder expects a code-signing file, as in your example as well, Anyway, I will test the patch provided. |
Great callout. I've added a unit test in the PR that throws Invalid Configuration when none of the required env var combinations are detected. I made the check occur after installing the nuget package provider and trusted signing module so that it is also covered in the CI tests. Good thing too, as GH runners use pwsh.exe, which differs from my VM of powershell.exe. There's some -Command differences between the two usages that I'm trying to iron out
|
OK with that patch I get this:
|
Yep, I discovered that as well. Got it working out locally, but haven't posted a patch for the updated code yet. Thanks for checking folks! |
Alright, got tests passing on GH runners covering invalid configuration + module installation in https://github.com/electron-userland/electron-builder/actions/runs/10998788786/job/30537499953?pr=8524
|
OK amazing, that is working perfectly now! |
Thanks for testing @MikeJerred ! Re:
|
Not sure what options you can put, or why someone would want to change it, so I would say put it as defaulted to SHA256 allowing overrides - as in your comment there. |
Merged PR with fixes #8524 Deploying the release now. It'll be in |
Already a step further in my case, it starts attempting signing now. But still get an error.
Must note I am trying this on a Windows on ARM (on a Mac with Parallels). So could be it influences some things... Update:
Powershell environment:
Update2: But then I get the message: Update3: After adding codeSigningAccountName (thus which is required), I get successfully to the signtool.exe execution. Also, might be very useful to add this resource to the documentation for Azure, as it is gold: https://melatonin.dev/blog/code-signing-on-windows-with-azure-trusted-signing/ |
@Bartel-C8 thank you for the deep debugging and report! Is this on local PC or a GH runner (or other platform)? I ask because my unit tests are not able to replicate the initialization setup error you're receiving on GH runner or on my local Parallels VM. (That being said, Parallels VM only needs to be set up once so there is the chance it already had the correct setup, but I don't recall ever setting Re: |
Released v25.1.6 with I'm going to close this issue since Azure Trusted Signing is now supported in electron-builder per the OP request and this thread is super long. If you encounter any issues from here, please open a new GH Issue and I'll be happy to take a look. |
Note, additional fix needed to be added in v26.0.0-alpha.4 (or you can add the values as custom to your current config in 25.1.8) Based off: https://github.com/Azure/trusted-signing-action?tab=readme-ov-file#timestamping-1
I missed this requirement during initial implementation. It wasn't ever specified in the Trusted Signing Docs, only as an optional param in the GH Action readme ¯_(ツ)_/¯. Sorry about that folks, just wanted to post a follow-up here to notify all to add these params to your config or update their electron-builder version (which just enforces via config that those are added)
|
Related note, has anyone tested out the auto-update flow? The auto-update signature verification is based on the publisher name in the signing certificate, but since Azure Trusted Signing has no local certificate, the |
Quick update on ☝️: I've added a required Summary: Since azure trusted signing occurs after the app-update.yml is created during I felt it was required because why wouldn't you want signing verification on auto-update if you're using the latest secure way to sign windows applications. ¯\(ツ)/¯ If it does not feel like it should be a required property, please comment here, open to the community's thoughts. Ideally, there'd be an automatic way to fetch the Publisher Name via some Azure Trusted service/command before signing a file, but I haven't been able to identify a way. Suggestions are welcome! Current workaround is to set |
Hey folks! First of all, thank you for getting this feature out there, you are awesome! I've been trying to get it working for hours now, no luck, so I'm all ears. The package I'm using is 25.1.8. Here's a pastebin with the logs I'm getting. Maybe I'm plumbing things the wrong way (even in Azure, lol). I'd love to have a conversation about this issue with someone, if there's anybody willing to lend a hand to an indie dev. Thank you. EDIT: eventually, I figured it out. I'll leave this here, maybe someone will need it in the future. I forgot to set up an IAM on the Azure side... Anyway this document is a good read that helped me solve the problem. |
I'm not sure where should I put the publisherName property?
Well, my real name contains accented characters in the publisherName just like this example. Throws an error:
|
In my case the publisherName is a level higher:
A good read for me was: https://melatonin.dev/blog/code-signing-on-windows-with-azure-trusted-signing/ But it could be that special characters are not handled that well? |
Yeah, the CN and O values are the same, with all that special character stuff. Maybe I just messed up my config JSON. Anyway, the last thing I want is to break my update mechanism. My app is out there already with paying customers. Anyway, having my installer signed is still better than having nothing. Validating sig on update would be a cherry on top but obviously I want to be 100% it won't break a thing. |
Yeah this is definitely still a Beta feature since I'm unable to test the full flow myself locally w/o paying a subscription with MS Azure as it'd also run in our CI flows |
I'm inquiring about the possibility of integrating Azure Trusted Signing within the Electron Builder workflow. Currently, my build process involves:
If you could provide guidance on how to integrate Azure Trusted Signing more seamlessly with Electron Builder or if there are plans to support this in the future, it would be greatly appreciated.
Thank you for your time and assistance.
The text was updated successfully, but these errors were encountered: