Keg: treat codesigning exceptions as warnings by mistydemeo · Pull Request #9040 · Homebrew/brew

mistydemeo · 2020-11-04T00:34:09Z

Have you followed the guidelines in our Contributing document?
Have you checked to ensure there aren't other open Pull Requests for the same change?
Have you added an explanation of what your changes do and why you'd like us to include them?
Have you written new tests for your changes? Here's an example.
Have you successfully run brew style with your changes locally?
Have you successfully run brew tests with your changes locally?
Have you successfully run brew man locally and committed any changes?

We've seen in the past that certain dylibs will cause the system codesign utility to trip. We don't need to consider this to be fatal in most cases, so we can downgrade these exceptions to warnings.

Requires Homebrew/ruby-macho#275 - we need to merge that and bump the ruby-macho version for this to be usable.

cc #8922.

This is an alternative to Homebrew#9040. In that case, we rely on ruby-macho to apply code signatures after changing ID or install_name. Here, we instead do that ourselves so that we can choose what OS to apply it on. Like in that PR, we swallow any failures so that we don't inadvertently break more exotic packages like OpenJDK (which caused issues in Homebrew#8922).

MikeMcQuaid · 2020-11-04T08:36:54Z

Library/Homebrew/os/mac/keg.rb

I think it would be ideal if this didn't do the code signing but it was done separately with MachO::Tools.codesign or similar. That allows us to catch the exception just for the code signing stage and generally separate the two. It'll also make it easier to have e.g. a DSL to enable/disable code-signing per-formula.

I'm of two minds about this. Let me explain...

Ideally, I think ruby-macho shouldn't "break" binaries. That is, from the gem's perspective (and not Homebrew's perspective), if we make any changes to a binary it should leave at the same level of correctness as where we started. Today, that's not the case: if a mach-o binary is signed, and we then modify that binary with ruby-macho, we leave it with an invalid code signature and the OS will refuse to run or load it until it gets resigned later. install_name_tool and friends automatically apply an ad-hoc code signature when modifying binaries and that's the behaviour I was looking to replicate in ruby-macho.

On the other hand, some of the considerations around codesigning are helped if we have access to Homebrew's knowledge about the host OS and the target OS - things that ruby-macho isn't aware of. Based on that, I can see the argument for having ruby-macho leave the actual call to perform signing up to Homebrew, even if the utility function lives in ruby-macho.

cc @woodruffw - I'd love to get your call here.

Yeh, that argument is convincing and I agree with it. I remove my objections, thanks @mistydemeo!

It'll also make it easier to have e.g. a DSL to enable/disable code-signing per-formula.

I think this would be nice but it does look like (please correct me if I'm wrong!) that codesign! is already a public API so we could call it directly if desired.

I think this would be nice but it does look like (please correct me if I'm wrong!) that codesign! is already a public API so we could call it directly if desired.

That is correct! It's already a public API and we can use it as desired.

mistydemeo · 2020-11-06T19:37:49Z

Now that we have ruby-macho 2.4.0 with the changes we rely on, I've incorporated it via brew vendor-gems and marked this PR as ready for review.

mistydemeo · 2020-11-06T19:59:23Z

Tested and working as expected on ARM macOS 11 and Intel macOS 10.15. Here's what a codesigning failure looks like, using openjdk as an example:

$ brew install openjdk
==> Downloading https://homebrew.bintray.com/bottles/openjdk-14.0.1.catalina.bottle.tar.gz
Already downloaded: /Users/mistydemeo/Library/Caches/Homebrew/downloads/b8c3f9d37365e3124933e42b4f06a1b732db5b50d33cb9f1449ab1dcad5c9de3--openjdk-14.0.1.catalina.bottle.tar.gz
==> Pouring openjdk-14.0.1.catalina.bottle.tar.gz
Warning: Failed to write a new code signature while changing
dylib ID of /usr/local/Cellar/openjdk/14.0.1/libexec/openjdk.jdk/Contents/MacOS/libjli.dylib
==> Caveats
For the system Java wrappers to find this JDK, symlink it with
  sudo ln -sfn /usr/local/opt/openjdk/libexec/openjdk.jdk /Library/Java/JavaVirtualMachines/openjdk.jdk

openjdk is keg-only, which means it was not symlinked into /usr/local,
because it shadows the macOS `java` wrapper.

If you need to have openjdk first in your PATH run:
  echo 'set -g fish_user_paths "/usr/local/opt/openjdk/bin" $fish_user_paths' >> ~/.config/fish/config.fish

For compilers to find openjdk you may need to set:
  set -gx CPPFLAGS "-I/usr/local/opt/openjdk/include"

==> Summary
🍺  /usr/local/Cellar/openjdk/14.0.1: 634 files, 320MB

We've seen in the past that certain dylibs will cause the system codesign utility to trip. We don't need to consider this to be fatal in most cases, so we can downgrade these exceptions to warnings. cc Homebrew#8922.

fxcoudert · 2020-11-06T22:28:15Z

When that warning occurs:

==> Pouring openjdk-14.0.1.catalina.bottle.tar.gz
Warning: Failed to write a new code signature while changing
dylib ID of /usr/local/Cellar/openjdk/14.0.1/libexec/openjdk.jdk/Contents/MacOS/libjli.dylib

It still means that we created a corrupted file, do I understand that correctly? i.e., in that case, libjli.dylib will not have a valid signature and will fail to run/be loaded.

mistydemeo · 2020-11-06T22:31:20Z

In this case the file is in the same state as we received it: it was not signed when it was built, and remains unsigned afterwards.

$ codesign --verbose -v ./MacOS/libjli.dylib
./MacOS/libjli.dylib: code object is not signed at all
In architecture: x86_64

fxcoudert · 2020-11-06T22:35:35Z

@mistydemeo but would we also treat a code signing error on a codesigned file as a warning, not an error? That seems wrong, given that on ARM Big Sur a non-codesigned file will not be functional

fxcoudert · 2020-11-06T23:10:01Z

Also: I’d rather we be conservative with code-signing, and only sign things that were signed to begin with. Or sign only on ARM Big Sur, where we know signatures are required.

Unless there is a clear benefit I don’t see, it seems quite invasive (and with potential for bugs, as we’re seeing) for no real gain

fxcoudert · 2020-11-07T14:20:01Z

I can confirm a failure while building Ruby with this PR applied:

==> ./configure --prefix=/usr/local/Cellar/ruby/2.7.2 --enable-shared --with-sitedir=/usr/local/lib/ruby/site_ruby --with-vendordir=
==> make
==> make install
==> /usr/local/Cellar/ruby/2.7.2/bin/ruby setup.rb --prefix=/private/tmp/ruby-20201107-2574-xenpdv/ruby-2.7.2/vendor_gem
Warning: Failed to write a new code signature while changing
dylib ID of /usr/local/Cellar/ruby/2.7.2/lib/libruby.2.7.dylib
Warning: The post-install step did not complete successfully
You can try again using `brew postinstall ruby`

It seems ruby_macho fails to set the signature for /usr/local/Cellar/ruby/2.7.2/lib/libruby.2.7.dylib, which I can check:

% codesign -v /usr/local/Cellar/ruby/2.7.2/lib/libruby.2.7.dylib
/usr/local/Cellar/ruby/2.7.2/lib/libruby.2.7.dylib: invalid signature (code or signature have been modified)
In architecture: arm64

The installed ruby is then nonfunctional, any attempt to run the binary will be killed.

I think this highlights that codesigning exceptions should be errors, not warnings, when the original file was codesigned.

mistydemeo · 2020-11-08T21:31:54Z

I think you're right. That being the case, I think this is going to need a little refactoring:

Apply codesigning only on macOS 11 Apple Silicon in order to avoid codesigning failures breaking packages on 10.15.
Rescue codesigning exceptions only so we can reraise them with more user-oriented error messages.

cc @MikeMcQuaid - this is a reversal of your and my earlier decisions here, but given the circumstances I think this is going to be the more user-friendly option. We can revisit this if the codesign bug is fixed.

MikeMcQuaid · 2020-11-09T09:30:01Z

Apply codesigning only on macOS 11 Apple Silicon in order to avoid codesigning failures breaking packages on 10.15.

Rescue codesigning exceptions only so we can reraise them with more user-oriented error messages.

cc @MikeMcQuaid - this is a reversal of your and my earlier decisions here, but given the circumstances I think this is going to be the more user-friendly option. We can revisit this if the codesign bug is fixed.

This is fine with me. Can we please, then, make the conditional code-signing logic be in Homebrew/brew rather than ruby-macho and make it a two-pass process? It's much nicer and easier to be able to change this logic as we decide to experiment with different ways of doing code-signing, reporting it to users, enabling it sometimes and not others etc.

MikeMcQuaid · 2020-11-09T09:37:07Z

Can we please, then, make the conditional code-signing logic be in Homebrew/brew rather than ruby-macho and make it a two-pass process?

Opened PR for part of this in Homebrew/ruby-macho#287

claui · 2020-11-11T16:20:38Z

Library/Homebrew/Gemfile.lock

    rubocop-sorbet (0.5.1)
      rubocop
-    ruby-macho (2.2.0)
+    ruby-macho (2.4.0)


Suggested change

ruby-macho (2.4.0)

ruby-macho (2.5.0)

2.5.0 has just landed, thanks @woodruffw!

mistydemeo · 2020-11-16T18:50:07Z

Closed by #9102.

mistydemeo force-pushed the allow_codesigning_to_fail branch from 6c10afb to 3e84dc1 Compare November 4, 2020 00:34

mistydemeo mentioned this pull request Nov 4, 2020

Keg: apply ad-hoc signature on 10.15 or later #9041

Closed

7 tasks

MikeMcQuaid reviewed Nov 4, 2020

View reviewed changes

mistydemeo force-pushed the allow_codesigning_to_fail branch from 3e84dc1 to a2a6a4e Compare November 6, 2020 19:36

mistydemeo marked this pull request as ready for review November 6, 2020 19:37

mistydemeo force-pushed the allow_codesigning_to_fail branch from a2a6a4e to d26fc17 Compare November 6, 2020 19:59

mistydemeo added 3 commits November 6, 2020 12:20

Keg: treat codesigning exceptions as warnings

966e433

We've seen in the past that certain dylibs will cause the system codesign utility to trip. We don't need to consider this to be fatal in most cases, so we can downgrade these exceptions to warnings. cc Homebrew#8922.

ruby-macho 2.4.0

7ed2a77

brew vendor-gems: commit updates.

2ee9aec

mistydemeo force-pushed the allow_codesigning_to_fail branch from d26fc17 to 2ee9aec Compare November 6, 2020 20:20

Update Tapioca definitions

151b0be

mistydemeo mentioned this pull request Nov 8, 2020

Codesigning on macOS 11 on Apple Silicon #9082

Closed

MikeMcQuaid mentioned this pull request Nov 9, 2020

macho: require codesign! call to be done explicitly. Homebrew/ruby-macho#287

Merged

claui suggested changes Nov 11, 2020

View reviewed changes

mistydemeo closed this Nov 16, 2020

BrewTestBot added the outdated PR was locked due to age label Dec 17, 2020

Homebrew locked as resolved and limited conversation to collaborators Dec 17, 2020

Uh oh!

Conversation

mistydemeo commented Nov 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MikeMcQuaid Nov 4, 2020

Choose a reason for hiding this comment

Uh oh!

mistydemeo Nov 5, 2020

Choose a reason for hiding this comment

Uh oh!

MikeMcQuaid Nov 5, 2020

Choose a reason for hiding this comment

Uh oh!

mistydemeo Nov 6, 2020

Choose a reason for hiding this comment

Uh oh!

mistydemeo commented Nov 6, 2020

Uh oh!

mistydemeo commented Nov 6, 2020

Uh oh!

fxcoudert commented Nov 6, 2020

Uh oh!

mistydemeo commented Nov 6, 2020

Uh oh!

fxcoudert commented Nov 6, 2020

Uh oh!

fxcoudert commented Nov 6, 2020

Uh oh!

fxcoudert commented Nov 7, 2020

Uh oh!

mistydemeo commented Nov 8, 2020

Uh oh!

MikeMcQuaid commented Nov 9, 2020

Uh oh!

MikeMcQuaid commented Nov 9, 2020

Uh oh!

claui Nov 11, 2020

Choose a reason for hiding this comment

Uh oh!

mistydemeo commented Nov 16, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

mistydemeo commented Nov 4, 2020 •

edited

Loading