fix(homebrew): codesign binary only on Intel macOS

[soerenkampschroer]

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

[ReenigneArcher]

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.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 11.14%. Comparing base (f418566) to head (726dc56).
Report is 93 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]

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]

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]

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]

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]

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.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

[soerenkampschroer]

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

[ReenigneArcher]

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.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 11.14%. Comparing base (f418566) to head (726dc56).
Report is 93 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]

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]

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]

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]

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]

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.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

[soerenkampschroer]

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

[ReenigneArcher]

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.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 11.14%. Comparing base (f418566) to head (726dc56).
Report is 93 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]

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]

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]

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]

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]

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.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

[soerenkampschroer]

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

[ReenigneArcher]

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.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 11.14%. Comparing base (f418566) to head (726dc56).
Report is 93 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]

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]

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]

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]

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]

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.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

[soerenkampschroer]

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

[ReenigneArcher]

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.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 11.14%. Comparing base (f418566) to head (726dc56).
Report is 93 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]

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]

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]

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]

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]

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.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

[soerenkampschroer]

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

[ReenigneArcher]

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.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 11.14%. Comparing base (f418566) to head (726dc56).
Report is 93 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]

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]

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]

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]

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]

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.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

[soerenkampschroer]

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

[ReenigneArcher]

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.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 11.14%. Comparing base (f418566) to head (726dc56).
Report is 93 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]

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]

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]

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]

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]

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.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

[soerenkampschroer]

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

[ReenigneArcher]

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.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 11.14%. Comparing base (f418566) to head (726dc56).
Report is 93 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]

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]

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]

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]

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]

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.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud