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

keg: add codesigning #9102

Merged
merged 1 commit into from
Nov 14, 2020
Merged

keg: add codesigning #9102

merged 1 commit into from
Nov 14, 2020

Conversation

fxcoudert
Copy link
Member

I tried to propose an implementation of the discussion in #9082
by building on @mistydemeo's #9041

  • Only codesign on Big Sur
  • Apply the known workaround if codesign fails the first time

Testing shows that this allows to build and install formulas on ARM, including the ones that trigger the codesign bug (like gettext and nettle).


Possible improvements:

  • only codesign if the macho file was previously signed? I don't know how we detect this
  • only codesign on ARM? I don't know if developer tools on Intel Big Sur codesign or not by default

@@ -31,4 +33,37 @@ def change_install_name(old, new, file)
EOS
raise
end

def apply_ad_hoc_signature(file)
return if MacOS.version < :big_sur
Copy link
Member

Choose a reason for hiding this comment

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

Do we want to also limit this to ARM for now?

Copy link
Member Author

Choose a reason for hiding this comment

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

I am waiting for feedback from @mistydemeo, who is checking whether Intel Big Sur codesigns by default or not.
If we want to limit to ARM, I don't know how to do in terms of Ruby calls. Can you suggest something?

Copy link
Member Author

Choose a reason for hiding this comment

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

Xcode 12.2, even on ARM Big Sur, does not codesign for Intel:

% clang -arch x86_64 a.c
% file a.out
a.out: Mach-O 64-bit executable x86_64
% codesign -v a.out
a.out: code object is not signed at all
In architecture: x86_64

So I suppose you're right, we should limit to Intel. Help is welcome. Looking at os/mac/mach.rb it looks like I should be able to simply call file.arch? But maybe it's not handled, as the code does not appear to include Arm variants in the list…

Copy link
Contributor

Choose a reason for hiding this comment

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

@fxcoudert Try Hardware::CPU.arm?, it’s in hardware.rb.

Copy link
Contributor

@claui claui left a comment

Choose a reason for hiding this comment

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

Relaying some feedback received via DM from the security team of a large-ish tech company (paraphrased, with my own comments added in italics):

  • Shelling out to install_name_tool is preferred as it will handle the architectures and everything fine

  • Re-signing already-signed artifacts may cause issues down the line if revocation is necessary

    • This refers to e. g. malware which is certificate-signed and the certificate is later revoked by Apple. We can fix this by re-signing only if the original signature is linker-signed
  • If you don't --force, linker signed signatures will be silently replaced and existing ones will not (with codesign returning an error), which might be a better paradigm

    • I fully agree
  • Patching keg-installed artifacts is probably breaking the existing signatures anyway and will require a re-sign

    • True, that’s one of the reasons why this PR exists

@fxcoudert
Copy link
Member Author

Shelling out to install_name_tool is preferred as it will handle the architectures and everything fine

That's not really touched by this PR, but Homebrew chose to use a native Mach-O library to do that.

Re-signing already-signed artifacts may cause issues down the line

Signed binaries are currently incompatible with our relocation system anyway: we manipulate them and therefore invalidate the signature.

If you don't --force, linker signed signatures will be silently replaced and existing ones will not

However, the binaries produced will then be invalid, because the signature won't match the content anymore. So we can't do that, because:

Patching keg-installed artifacts is probably breaking the existing signatures anyway

Yes.

@claui
Copy link
Contributor

claui commented Nov 12, 2020

Fair points @fxcoudert.

@fxcoudert
Copy link
Member Author

@MikeMcQuaid I've limited to ARM, as requested. I've been testing this (and earlier versions) for a week now, building many formulas with it (that are broken without)

Copy link
Contributor

@claui claui left a comment

Choose a reason for hiding this comment

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

Been using this patch actively since last night and my manual tests have shown this works as expected, both on Rosetta and native Apple Silicon.

$ codesign -vv /opt/dunnobrew/opt/nettle/bin/nettle-hash
/opt/dunnobrew/opt/nettle/bin/nettle-hash: code object is not signed at all
In architecture: x86_64
$ codesign -vv /opt/homebrew/opt/nettle/bin/nettle-hash
/opt/homebrew/opt/nettle/bin/nettle-hash: valid on disk
/opt/homebrew/opt/nettle/bin/nettle-hash: satisfies its Designated Requirement

@claui
Copy link
Contributor

claui commented Nov 13, 2020

@MikeMcQuaid @mistydemeo Would love to have one of you look at it for good measure.

@claui claui merged commit e945b1c into Homebrew:master Nov 14, 2020
@fxcoudert fxcoudert deleted the sign branch November 14, 2020 09:43
@BrewTestBot BrewTestBot added the outdated PR was locked due to age label Dec 14, 2020
@Homebrew Homebrew locked as resolved and limited conversation to collaborators Dec 14, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants