macOS Code Signing and Notarization by halprin · Pull Request #669 · DescentDevelopers/Descent3

halprin · 2025-02-24T01:01:26Z

Pull Request Type

Description

The macOS binaries are now code signed and notarized.

There are two ways this is accomplished.

The hog files that contain dynamic libraries. These libraries must be signed before packaging into the hog file (because you can't just sign the hog file), so they are signed during the build. This includes d3-osx.hog and everything in the online folder.
The main application and raw binaries in the netgames folder. These binaries are signed after the build using a helper GitHub action. This helper GitHub action also packages everything into a DMG disk image and notarizes the disk image. A DMG is an established package format and natively supports notarization, unlike a zip file.

Code signing and notarization only happens on push to the main branch, not in a pull request. Secrets can't be accessed in a pull request from a fork, but I also chose to refactor the GitHub action workflows to explicitly not pass in secrets into the pull request workflow to make it obvious. Let me know if we don't like this, but I thought the explicit approach was better than implicit. We can also leverage this approach in the future for versioned releases.

Updated the documentation to remove the work-arounds needed when the application was unsigned.

If you want to inspect a test build, you can see this build in my fork. You can't depend on the build on this PR because, as mentioned above, we aren't signing and notarizing for PR builds.

Related Issues

Fixes #540.
Fixes #559.

Screenshots (if applicable)

Checklist

I have tested my changes locally and verified that they work as intended.
I have documented any new or modified functionality.
I have reviewed the changes to ensure they do not introduce any unnecessary complexity or duplicate code.
I understand that by submitting this pull request, I am agreeing to license my contributions under the project's license.

Additional Comments

…arization

…e into installed folder

…d readme

tophyr

Thank you, thank you THANK YOU!!! This will really help the user experience on Mac. I noted some threat-model risks that we should mitigate, but I don't think they're critical to do before merging this.

.github/workflows/build.yml

tophyr · 2025-02-25T18:00:01Z

.github/workflows/build.yml

    runs-on: ${{ matrix.os.runner }}

+    env:
+        MACOS_SIGNING: ${{ secrets.MACOS_SIGNING_IDENTITY != '' }}


nit: might be better to include the os preset check in here as well? then the if clauses in the following changes could check just one master variable. thoughts?

Suggested change

MACOS_SIGNING: ${{ secrets.MACOS_SIGNING_IDENTITY != '' }}

MACOS_SIGNING: ${{ matrix.os.preset == 'mac' && secrets.MACOS_SIGNING_IDENTITY != '' }}

We can't lump them together because sometimes we need to build for non-Mac, build for the Mac but not sign, and build for the Mac and sign. If we combined the variables, then we couldn't sometimes build for the Mac but not sign or sometimes build for the Mac and sign.

It would just be MACOS_SIGNING=false in those cases, wouldn't it? I'm just noticing that all three times MACOS_SIGNING is referenced, it's also &&'ed with the os.preset == 'mac' check. If we move the os.preset == 'mac' check into the MACOS_SIGNING variable, we can remove it from the three usage sites and prevent mixups in the future if someone alters this workflow to add a new usage but forgets to check the OS.

Oh, I see. Hmmm. But even with this change we would continue to use if: ${{ matrix.os.preset == 'mac' }} like when we call Install macOS Rosetta 2 and a couple others places, right?

Oh, I see. Hmmm. But even with this change we would continue to use if: ${{ matrix.os.preset == 'mac' }} like when we call Install macOS Rosetta 2 and a couple others places, right?

I would imagine, yeah. Adding the check here would just be for fully-encapsulating the standalone answer to "should i perform mac signing"

.github/workflows/build.yml

tophyr · 2025-02-25T18:16:42Z

.github/workflows/ci.yml

+  build:
+    name: Build for PR
+    uses: ./.github/workflows/build.yml
+    # explicitly not passing secrets into the build


double-checking: pull requests originating from outside the Descent3 repository will never get access to secrets, no matter what - correct? ie, even if the PR modifies this file, or even if someone merges a modification to this file?

(is it even possible to submit a PR to yourself? suppose i could try that out, but kinda irrelevant either way)

I'll do an actual live test of this tomorrow.

The live test was a success. I confirmed GitHub's documentation to be correct. A PR from a fork has no access to the secrets in the base repository. Even if the fork PR changes the GitHub Action and tries to reference the secrets and exfiltrate them, nothing is accessed.

USAGE.md

Lgt2x

I tested your artifact, it works great! That's some seriously cool work you did there :)

I invited you to the github org so you can push the secrets you need

.github/workflows/cd.yml

Lgt2x · 2025-02-25T19:19:29Z

.github/workflows/build.yml

          CC: ${{ matrix.os.cc }}
          CXX: ${{ matrix.os.cxx }}
-        run: cmake --preset ${{ matrix.os.preset }} -DBUILD_TESTING=ON -DENABLE_LOGGER=ON -DFORCE_PORTABLE_INSTALL=ON -DBUILD_EDITOR=ON -DUSE_EXTERNAL_PLOG=ON
+        run: cmake --preset ${{ matrix.os.preset }} -DCODESIGN_IDENTITY=${{ secrets.MACOS_SIGNING_IDENTITY }} -DBUILD_TESTING=ON -DENABLE_LOGGER=ON -DFORCE_PORTABLE_INSTALL=ON -DBUILD_EDITOR=ON -DUSE_EXTERNAL_PLOG=ON


The CODESIGN_IDENTITY CMake cache variable should be declared as such in the top-level CMakeLists.txt: set(CODESIGN_IDENTITY ... CACHE STRING ...) like we do for USE_VCPKG for example. It should also be documented in BUILD.md

Fixed. CMake isn't my forte, so let me know if I can do this better.

Oooo, maybe not fixed. In fact, the build is failing. Sad.

I tried futzing with it but couldn't get it working. I'll try again tomorrow.

I'll look into it tonight and try to help you out

see suggestions below

Thanks for the help! Fixed.

Lgt2x · 2025-02-25T19:20:21Z

netcon/descent3onlineclient/CMakeLists.txt

 target_include_directories(Descent3_Online_TCP_IP PRIVATE ${SDL3_INCLUDE_DIRS})
 if(CMAKE_SYSTEM_NAME STREQUAL "Darwin")
  set_target_properties(Descent3_Online_TCP_IP PROPERTIES SUFFIX ".dylib")
+  if(DEFINED CODESIGN_IDENTITY AND NOT "${CODESIGN_IDENTITY}" STREQUAL "")


We could make that couple lines a CMake function or macro that takes the target name as argument

You can define a function like this in the top level CMakelists:

set(CODESIGN_IDENTITY "" CACHE STRING "Sets the macOS code signing identity. If set to something besides the empty string, then the dynamic libraries put into the hog files will be signed using this identity.") function (macos_sign target) message ("target: ${target}") if(DEFINED CODESIGN_IDENTITY AND NOT "${CODESIGN_IDENTITY}" STREQUAL "") message(STATUS "Code signing ${target}") add_custom_command(TARGET ${target} POST_BUILD COMMAND codesign --verbose --sign "${CODESIGN_IDENTITY}" --force --timestamp --deep -o runtime $<TARGET_FILE:${target}>) endif() endfunction()

and use it as macos_sign("Parallax_Online") for instance

I hope this helps

Thanks for the help! Fixed.

tophyr

Please use the https://github.com/DescentDevelopers/import-codesign-certs and https://github.com/DescentDevelopers/macos-sign-package-notarize actions instead of external ones, now that @Lgt2x has forked them.

Lgt2x · 2025-02-26T20:17:06Z

CMakeLists.txt

  set(CMAKE_BUILD_TYPE "Debug" CACHE STRING "default build type")
 endif()

+set(CODESIGN_IDENTITY "" CACHE STRING "Sets the macOS code signing identity.  If set to something besides the empty string "", then the dynamic libraries put into the hog files will be signed using this identity.")


this does not work because of the double quotes inside of the documentation string "", CMake does not like that

Thanks for the help there!

Lgt2x · 2025-02-27T08:49:57Z

Good for me, are the secrets all set on this repository so we can merge @halprin ?

halprin · 2025-02-27T15:09:46Z

We're ready to merge!

halprin added 10 commits February 22, 2025 17:04

Code sign the macOS binaries that go into d3-osx.hog

ea78f8d

Code sign the netcon clients for macOS

918343e

Update Build GitHub Action to support macOS signing, package, and not…

5cbeb9a

…arization

Update secret name

b982760

try using glob syntax to sign everything in netgames

fb34cc2

Clean the installed folder after making disk image and move disk imag…

b4827b4

…e into installed folder

Remove all references to the macOS binary not being signed in USAGE.m…

c509705

…d readme

Split the PR workflow from the main build workflow to be explicit

bfdf9f8

Test passing in secrets into PR

b893489

Remove test of putting secrets into PR workflow

be9e6fa

Lgt2x requested review from Lgt2x and tophyr February 24, 2025 07:42

tophyr approved these changes Feb 25, 2025

View reviewed changes

Lgt2x reviewed Feb 25, 2025

View reviewed changes

tophyr requested changes Feb 25, 2025

View reviewed changes

halprin added 3 commits February 25, 2025 20:40

Try DescentDevelopers version of GitHub Actions

301380c

test fake secret to test

7dd02a4

Remove fake secret, test was successful

6da8ddd

tophyr approved these changes Feb 26, 2025

View reviewed changes

Set CODESIGN_IDENTITY in the top level CMakeLists.txt

c1a84a3

Lgt2x reviewed Feb 26, 2025

View reviewed changes

halprin added 5 commits February 26, 2025 14:44

Create macos_sign CMake function

e1e8da4

Update BUILD.md wih the new description of CODESIGN_IDENTITY

3a4ac89

Test using a combined variable

8d00e9f

Remove test secret

c4b20fe

Indent only two spaces

52724da

Lgt2x approved these changes Feb 27, 2025

View reviewed changes

halprin merged commit d48b0f1 into DescentDevelopers:main Feb 27, 2025
10 checks passed

halprin deleted the macos-code-sign branch February 27, 2025 15:52

mwestphal mentioned this pull request Jun 18, 2025

Sign macOS app with a certificate f3d-app/f3d#2303

Open

	MACOS_SIGNING: ${{ secrets.MACOS_SIGNING_IDENTITY != '' }}
	MACOS_SIGNING: ${{ matrix.os.preset == 'mac' && secrets.MACOS_SIGNING_IDENTITY != '' }}

Conversation

halprin commented Feb 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Type

Description

Related Issues

Screenshots (if applicable)

Checklist

Additional Comments

Uh oh!

tophyr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tophyr Feb 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tophyr Feb 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

halprin Feb 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Lgt2x left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Lgt2x Feb 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tophyr left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Lgt2x commented Feb 27, 2025

Uh oh!

halprin commented Feb 27, 2025

Uh oh!

halprin commented Feb 24, 2025 •

edited

Loading

tophyr Feb 26, 2025 •

edited

Loading

tophyr Feb 26, 2025 •

edited

Loading

halprin Feb 26, 2025 •

edited

Loading

Lgt2x Feb 26, 2025 •

edited

Loading

tophyr left a comment •

edited

Loading