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(homebrew): codesign binary only on Intel macOS #3348

Merged
merged 3 commits into from
Nov 1, 2024

Conversation

soerenkampschroer
Copy link
Contributor

@soerenkampschroer soerenkampschroer commented Oct 31, 2024

Description

Moving binary after build and adding codesign for macOS as per issue #3340

Screenshot

Issues Fixed or Closed

Issue #3340

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Dependency update (updates to dependencies)
  • Documentation update (changes to documentation)
  • Repository update (changes to repository files, e.g. .github/...)

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have added or updated the in code docstring/documentation-blocks for new or existing methods/components

@soerenkampschroer soerenkampschroer changed the title move binary after build and codesign (conditional to macOS) fix(homebrew): move binary after build and codesign (conditional to macOS) Oct 31, 2024
@ReenigneArcher
Copy link
Member

FYI, in the future you don't need to close your PR and open a new one to make fixes. You can just push your changes to your PR branch and it will be picked up.

@ReenigneArcher ReenigneArcher changed the title fix(homebrew): move binary after build and codesign (conditional to macOS) fix(homebrew): move binary after build and codesign (macOS) Oct 31, 2024
@ReenigneArcher ReenigneArcher enabled auto-merge (squash) October 31, 2024 21:33
Copy link

codecov bot commented Oct 31, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 11.14%. Comparing base (f418566) to head (726dc56).
Report is 1 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff            @@
##           master    #3348    +/-   ##
========================================
  Coverage   11.14%   11.14%            
========================================
  Files          99       99            
  Lines       17184    17184            
  Branches     8008     8008            
========================================
  Hits         1916     1916            
+ Misses      12722    12581   -141     
- Partials     2546     2687   +141     
Flag Coverage Δ
Linux 8.45% <ø> (ø)
Windows 5.22% <ø> (ø)
macOS-13 13.63% <ø> (-0.02%) ⬇️
macOS-14 12.63% <ø> (ø)

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

see 22 files with indirect coverage changes

@cathyjf
Copy link
Contributor

cathyjf commented Oct 31, 2024

What is this intended to fix? The issue of having to re-grant permissions for each build of sunshine will not be fixed by this, if that's the intent. That issue occurs because the ad hoc code signature changes each time the underlying binary changes; it's not because the filename is changing. To avoid having to remove and re-grant the permission each build, it's necessary to sign each build with the same Apple Developer ID certificate as the previous build, not an ad hoc certificate. Homebrew doesn't provide a way to do this, so the issue of having to remove and re-grant permissions cannot be fixed very easily.

@cathyjf cathyjf mentioned this pull request Oct 31, 2024
2 tasks
@cathyjf
Copy link
Contributor

cathyjf commented Oct 31, 2024

Also, Homebrew already signs all code it builds, at least on arm64. This pull request is not the correct solution to whatever it is trying to fix.

@cathyjf
Copy link
Contributor

cathyjf commented Oct 31, 2024

The actual solution to this general problem is that Sunshine should be distributed as a proper app bundle, and signed by some trusted certificate (perhaps owned by a trusted maintainer) as part of the GitHub build process. Then the homebrew formula should be replaced by a cask that just installs the trusted binary. This will solve the following problems:

  1. Permissions will not need to be removed and re-granted each time Sunshine updates any code.
  2. Sunshine will not be a gaping vulnerability on the host machine. Currently, once you grant Sunshine any permissions, you're actually granting those permissions to all programs, because any program can trivially inject code into the Sunshine process. To avoid this, it's necessary for Sunshine and all its dependencies to use the hardened runtime, and be distributed as an app bundle.

@cathyjf
Copy link
Contributor

cathyjf commented Oct 31, 2024

To summarize my comments, everything already works as well as it can be expected to work on arm64 because Homebrew already signs code on arm64. It apparently does not sign code on Intel. As a stopgap for Intel users, I suppose this pull request is fine, but I recommend that it be made specific to Intel to avoid conflicting with Homebrew's built in ad hoc codesigning. You can check for the architecture in the Homebrew formula.

Long term, Sunshine should be distributed as an app bundle, but I think it's valid to have a short term fix for Intel users.

@soerenkampschroer
Copy link
Contributor Author

Thank you so much for clearing that up @cathyjf that makes sense! I am running Intel, so that's probably why I needed to sign it myself. I wasn't aware about brew signing it only for arm64. I'll look into how to make it conditional to Intel and update the pull request accordingly. I'll also remove the line copying the binary.

Copy link

sonarqubecloud bot commented Nov 1, 2024

@ReenigneArcher ReenigneArcher changed the title fix(homebrew): move binary after build and codesign (macOS) fix(homebrew): codesign binary only on Intel macOS Nov 1, 2024
@ReenigneArcher ReenigneArcher enabled auto-merge (squash) November 1, 2024 14:11
@ReenigneArcher ReenigneArcher merged commit 9662f05 into LizardByte:master Nov 1, 2024
36 checks passed
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