-
-
Notifications
You must be signed in to change notification settings - Fork 9.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
extend/os/mac/keg: codesign on Intel if invalid signature #15903
Conversation
@@ -28,7 +28,11 @@ def binary_executable_or_library_files | |||
|
|||
def codesign_patched_binary(file) | |||
return if MacOS.version < :big_sur | |||
return unless Hardware::CPU.arm? | |||
|
|||
unless Hardware::CPU.arm? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this should be attempted regardless to CPU?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeh, I was wondering if there would be any downside to unconditionally code-signing patched binaries?
Vaguely relatedly: I wonder if it would ever be possible to:
- avoid patching/rewriting binaries installed into the default prefix (particularly
cellar :any
orcellar :any_no_relocation
- codesign binaries at CI time
CC @woodruffw as we've talked about this recently too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I see no reason not to attempt signature verification on other archs 🙂
And yep, I think it'd be possible to do things "right" here and push codesigning into CI (with the assumption that 99% of users wouldn't need to perform any relocations and could use the CI-supplied signature.) The main challenges that I can think of these are mostly logistical ones (e.g. around ensuring that the signing step remains secure.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeh, I was wondering if there would be any downside to unconditionally code-signing patched binaries?
Intel doesn't actually require dev tools installed, so we could be breaking existing setups there. If we implement codesigning in Ruby or something, this downside will disappear.
codesign binaries at CI time
This only works if there is no clientside modifications (e.g. relocation). The code here already does apply to CI time too, so if there's only CI-time modifications then codesigning will only apply there.
The number of formula we ship that's already codesigned on Intel is very small. The only known case is Qemu, which only recently broke as it didn't have any binary modifications until #15571 (and that change only applies on the CI side, so only took affect in a recent formula update).
With that said, I think it would be good idea to ship with the smallest necessary scope now (i.e. only codesign on Intel if there's an existing code signature that's been broken) so we can rebuild Qemu. Then we can have a bigger discussion on if we want anything more in the short/medium/long-term.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR! Requesting some other reviews.
@@ -28,7 +28,11 @@ def binary_executable_or_library_files | |||
|
|||
def codesign_patched_binary(file) | |||
return if MacOS.version < :big_sur | |||
return unless Hardware::CPU.arm? | |||
|
|||
unless Hardware::CPU.arm? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeh, I was wondering if there would be any downside to unconditionally code-signing patched binaries?
Vaguely relatedly: I wonder if it would ever be possible to:
- avoid patching/rewriting binaries installed into the default prefix (particularly
cellar :any
orcellar :any_no_relocation
- codesign binaries at CI time
CC @woodruffw as we've talked about this recently too.
Is there any blocker to get this merged ? 🙏 |
@@ -28,7 +28,11 @@ def binary_executable_or_library_files | |||
|
|||
def codesign_patched_binary(file) | |||
return if MacOS.version < :big_sur |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This OS conditional can probably be removed, arm64 can't be < Big Sur anyway.
Though we should check if the commands below haven't changed over the years.
Given this is breaking things for a number of people, and the reviews so far have only been about expanding scope, I'll merge this now given it already covers the scope necessary to fix Qemu. |
Thanks @cho-m! |
Anyone know situation with bottle using absolute path vs rewriting to I checked new I had tested this commit using |
Thank you for working on this. Could you take a look? |
brew style
with your changes locally?brew typecheck
with your changes locally?brew tests
with your changes locally?Attempt at fixing issue mentioned in Homebrew/homebrew-core#140244.
Due to enabling
HOMEBREW_RELOCATABLE_INSTALL_NAMES=1
, RPATH modifications are invalidating code signatures added by upstream installation script.Possible change here to
codesign
if we detect invalid signature on Intel binary. Alternative could be on formula-by-formula basis, though harder to catch problems before distribution to users.Snippet of debug output on
qemu
: