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

fix(bundler): sign the exe before the bundler step #7487

Merged
merged 4 commits into from
Aug 8, 2023

Conversation

FabianLars
Copy link
Member

https://discord.com/channels/616186924390023171/1010253864110407800

This way there's better support for standalone exe or in case devs create their own bundle formats.

At first i wanted to add that to the cli but thought that'd be a bit awkward since all the signing happens in the bundler and also in case we really end up making the bundler a more general thing for other projects to use, but i'm fine with changing that again.

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Docs
  • New Binding issue #___
  • Code style update
  • Refactor
  • Build-related changes
  • Other, please describe:

Does this PR introduce a breaking change?

  • Yes, and the changes were approved in issue #___
  • No

Checklist

  • When resolving issues, they are referenced in the PR's title (e.g fix: remove a typo, closes #___, #___)
  • A change file is added if any packages will require a version bump due to this PR per the instructions in the readme.
  • I have added a convincing reason for adding this feature, if necessary

Other information

@FabianLars FabianLars requested a review from a team as a code owner July 24, 2023 12:01
#[cfg(target_os = "windows")]
{
let main_binary = settings
.binaries()
Copy link
Member

Choose a reason for hiding this comment

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

should we take this opportunity to sign all binaries instead of just the main one?

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah, why not. gonna look into that asap

Copy link
Member Author

@FabianLars FabianLars Jul 24, 2023

Choose a reason for hiding this comment

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

2 Questions:

  • Do all the binaries in settings.binaries need to be signed? I don't know what binaries it bundles other than the main app
  • I think we need a setting to skip signing sidecars (or some of them) in case they were already signed beforehand by a third party or something idk

Copy link
Member

Choose a reason for hiding this comment

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

it bundles all binaries defined in the Cargo manifest. The sidecars are in a separate getter, external_binaries.

Copy link
Member Author

@FabianLars FabianLars Jul 24, 2023

Choose a reason for hiding this comment

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

it bundles all binaries defined in the Cargo manifest

So i assume we care about them? aka they need to be signed too? I can't think of usecase rn nvm, yeah we should sign it.

That then leaves us with the sidecar skip setting.

Copy link
Member

Choose a reason for hiding this comment

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

I guess no one is even using this otherwise it would probably have issues.

It's pretty much like a sidecar, but written in Rust in the same Cargo project.

Copy link
Member

Choose a reason for hiding this comment

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

Like how some projects implement the updater as a separate process. But I don't remember seeing anyone using it (except whoever requested this during our alpha, I think it was like a local server/client app).

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry for the delay, the change is windows-only for now, i'll try to at least add the settings.binaries for macos but i'm unsure how to handle sidecars. Mainly how to prevent re-signing already signed binaries and if they still have to be individually notarized or not and if that works if the sidecar was signed by a third party.

Anyway, here is what i added on Windows:
All the binaries in settings.binaries are code signed before the wix/nsis bundlers are invoked.
All the sidecar binaries are also code signed before the wix/nsis bundlers are invoked just to keep the logic together and to not have to duplicate it. The sidecar binaries get treated differently since we use signtool to check for existing signatures. I added an env var to skip that check and forcefully sign them.

lemme know what you think

@FabianLars
Copy link
Member Author

Posting this as a normal ocmment instead of an answer for better visibility

Okay, nevermind about the macos changes, they need a bit more exploration which i think is outside the scope of this PR:

We currently use the --deep option which should already make it sign all the binaries in the app bundle, but there are 2 issues with that.

  1. It doesn't seem to actually work because [bug] Sidecar not working after signing app - MAC OS #6888 wouldn't be a thing then, and
  2. all resources i found heavily discourage the use of --deep and instead recommend a more manual approach.

Lastly, there's an issue with how we handle plist files. Currently we apply it to all signed files and not just the main binary, which also seems to be frowned upon. This behavior is currently inherited from the --deep option but still something we have to keep in mind when we change the approach. Then we'll need to discuss whether we're okay with that or if we want to have a more granular (sidecar specific) plist appoach


I am interested in working on that, but not until i have a working macos setup 🙃

So in that context i consider this PR ready for review.

@lucasfernog lucasfernog merged commit cb1d416 into 1.x Aug 8, 2023
@lucasfernog lucasfernog deleted the early-windows-code-signing branch August 8, 2023 18:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: 🔎 In audit
Development

Successfully merging this pull request may close these issues.

2 participants