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

Explicitly add ad hoc code signing on macOS #5650

Merged
merged 6 commits into from Jan 31, 2022

Conversation

dyfer
Copy link
Member

@dyfer dyfer commented Dec 21, 2021

Fixes #5603

On macOS all binaries needs to be signed when run on M1 hardware (arm64 / aarch64)
When building, the binaries are signed ad hoc, however it turns out that macdeployqt, as well as manipulations with install_name_tool invalidate these signatures.
I propose the solution that runs ad hoc signing after the macdeployqt step.

I'm open to suggestions whether this is the desired solution and if there's a better name for the introduced CMake flag.

The PR includes the following changes:

  • A new option is added to CMakeLists.txt to trigger ad-hoc codesigning after building (SC_CODESIGN_AFTER_DEPLOY, defaults to ON, macOS only)
  • The following components are signed: binaries SuperCollider, sclang, scsynth, supernova, all .framework and .dylib files in Contents/Frameworks/, DiskIO_UGens plugins, and SuperCollider.app itself (last).
    • SuperCollider binary is signed with the --deep flag to avoid code object is not signed at all error; signing other files is done without that flag
    • Signing DiskIO_UGens was needed due to a rather obscure issue with codesigning - the servers would not boot, but only after incremental builds, while running fine on clean builds. Signing this specific ugen file seems to have fixed that. I made a note of it in the commit message.
  • I added --deep flag to build-time codesigning for the SuperCollider target. This seems to me like a more proper solution to [build] MacOS incremental builds broken #4664 than turning off build-time codesigning altogether.
    • Since the incremental builds do work now with the build-time codesigning turned on, I decided to turn OFF SC_DISABLE_XCODE_CODESIGNING by default, as I don't think it's needed anymore.

Purpose and Motivation

Types of changes

  • Bug fix

To-do list

  • Code is tested
  • All tests are passing
  • Updated documentation
  • This PR is ready for review

@dyfer dyfer added os: macOS comp: build CMake build system labels Dec 21, 2021
@dyfer dyfer force-pushed the topic/macos-codesigning-arm64 branch 2 times, most recently from b30e9c6 to 8f2e04d Compare December 21, 2021 10:13
@dyfer dyfer marked this pull request as draft January 10, 2022 21:34
@joshpar
Copy link
Member

joshpar commented Jan 11, 2022

I think this is probably going to be the way to go. However, I think this PR should also include changes to the Mac build notes.

@dyfer
Copy link
Member Author

dyfer commented Jan 11, 2022

My original implementation turned out to be faulty, since I was trying to gather a list of files in the installation location before they were created.

Subsequently I went through 3 different iterations of possible solutions (adding cmake commands to the string executed at the end, using ls to gather files and pass that to codesign using xargs and finally using find to get a list of files) with TONS of escaping, and I think the current solution is reasonable. I'll clean up the PR and add note in macOS build instructions before marking this as ready for review.

Failures in the CI seem unrelated to this PR BTW

@dyfer
Copy link
Member Author

dyfer commented Jan 11, 2022

I am going to also investigate adding the "linker-signed" flag which might solve some of the problem without re-signing. https://gitlab.kitware.com/cmake/cmake/-/issues/21854

@dyfer dyfer force-pushed the topic/macos-codesigning-arm64 branch from 07bade0 to b01eb4b Compare January 11, 2022 21:24
@dyfer
Copy link
Member Author

dyfer commented Jan 11, 2022

I am going to also investigate adding the "linker-signed" flag which might solve some of the problem without re-signing. https://gitlab.kitware.com/cmake/cmake/-/issues/21854

This turns out to be available only on macOS 11+ and I'm not sure if it would fix the issue with macdeployqt touching binaries (to be tested though - I'm not on M1 currently).

In the meantime I think I found a more proper solution to #4664 - original solution was to disable codesigning altogether; that didn't seem like the best idea to me and I found that running codesign --deep on one binary (SuperCollider itself, i.e. the IDE) solves the original problem. For now I'm putting this change in this PR but maybe we'll want to consider it separately?

Finally, I don't know how/what to document here. This doesn't seem to change anything about building locally (aside from making it work on M1) and AFAIU does not change anything about signing the releases either.

Before this PR local builds always had invalid signatures. While code signatures for local builds were not required to run them on earlier macOS systems (before Big Sur and M1), making this change for all builds seems fine to me, as it results in binaries with valid (albeit still ad-hoc) signatures.

@dyfer dyfer force-pushed the topic/macos-codesigning-arm64 branch from b01eb4b to 933c380 Compare January 12, 2022 23:07
@dyfer
Copy link
Member Author

dyfer commented Jan 12, 2022

I am happy with this PR now. I updated the first comment to give a clear description of the current changes.

Here's what changed from the previous version:

  • codesigning step a little more granular - codesign --deep is only run on Contents/MacOS/SuperCollider, while all other files are signed without the --deep flag
    • I did this since I've come across information that using the --deep flag unnecessarily is not recommended
  • I did investigate using -o linker-signed flag for codesigning, but eventually did not include it here
    • this was only relevant for libfftw3f.3.dylib, which we manually correct with install_name_tool after assembling the app
    • it would result in adding a re-signing step before running install_name_tool, but we re-sign all the libraries at the end anyway so I decided this would be superfluous
    • the only downside of the current solution is that we get a warning towards the end of the build process: install_name_tool: warning: changes being made to the file will invalidate the code signature in: (...)/supercollider/build/Install/SuperCollider/SuperCollider.app/Contents/Frameworks/libfftw3f.3.dylib
  • I've added codesigning for the DiskIO_UGens plugin. Other plugins don't seem to need to be re-signed, but this plugin does, however the issue only transpires on incremental builds. Originally I observed that that the server would not boot after an incremental build, unless codesigning in the build process was disabled. Eventually I tracked this down to this one plugin that needed to be re-signed in order to make incremental builds functional again (without disabling the codesigning in the build process)

@dyfer dyfer marked this pull request as ready for review January 12, 2022 23:51
@dyfer dyfer marked this pull request as draft January 13, 2022 00:02
@dyfer
Copy link
Member Author

dyfer commented Jan 13, 2022

Spoke too soon, something is wrong...

@dyfer
Copy link
Member Author

dyfer commented Jan 13, 2022

The issue was that the build wouldn't open after moving to another computer (the message was "SuperCollider" is damaged and can't be opened. (...) i.e. there was no option to "open anyway" even after right-click-open).

The missing part was signature on the app bundle itself (codesign --force --sign - SuperCollider.app). With the corrected signing step, the app can be opened again.

It is worth noting, that the message on Big Sur has changed because the app is signed. Previously it was "macOS can't verify the developer of "SuperCollider". With ad-hoc signed binaries the message is "SuperCollider" can't be opened because Apple cannot check it for malicious software. Do we prefer one message over the other? I.e. should ad-hoc signing be turned ON in the CI (current behavior in this PR, resulting in the latter message) or OFF (behavior from before this PR)? I personally don't think there's much difference and would leave the signing ON.

@dyfer dyfer marked this pull request as ready for review January 13, 2022 01:15
Note: signing DiskIO_UGens is needed for servers from incremental builds to start properly on arm64 (M1) hardware
@dyfer dyfer force-pushed the topic/macos-codesigning-arm64 branch from 0123bf7 to 531324e Compare January 13, 2022 01:32
@dyfer
Copy link
Member Author

dyfer commented Jan 30, 2022

It turned out, that I needed to sign all the ugens after all

@dyfer dyfer merged commit 908d88c into supercollider:develop Jan 31, 2022
@dyfer dyfer deleted the topic/macos-codesigning-arm64 branch January 31, 2022 02:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
comp: build CMake build system os: macOS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Codesigning issues on macOS with M1 (ARM64) processors
2 participants