Sign and notarize macOS binaries #514

ldennington · 2022-06-28T19:33:59Z

Update macOS portion of build-git-installers workflow to add signing and notarization. A few notes reviewers may find helpful:

This change removes our dependency on the derrickstolee/git_osx_installer repo by migrating the Makefile and scripts we need into Microsoft Git's .github directory (or a subdirectory, if applicable). The following are notes on each of these files:
1. .github/Makefile: This file was changed substantially (i.e. TODO comments, unneeded variables, and unneeded tasks were removed). Accordingly, it merits a robust review.
2. The following files were taken as-is from the git_osx_installer repo and should not require a detailed review:
  - .github/macos-installer/assets/etc/gitconfig.default
  - .github/macos-installer/assets/etc/gitconfig.osxkeychain
  - .github/macos-installer/assets/git-components.plist
  - .github/macos-installer/assets/uninstall.sh
  - .github/macos-installer/assets/scripts/postinstall
  - .github/scripts/symlink-git-hardlinks.rb
The ubuntu_build task was updated to also use the signing scripts added for the macOS workflow, allowing for removal of the sign-debian-packages.py script (which had hard-coded Linux values rather than OS-specific values passed as parameters).
Compilation of contrib/credential/osxkeychain failed without the small change I made in contrib/credential/osxkeychain/git-credential-osxkeychain.c.

Finally, here is the latest workflow run in my fork testing these changes.

derrickstolee · 2022-06-29T13:18:45Z

This change removes our dependency on the derrickstolee/git_osx_installer repo by migrating the Makefile and scripts we need into Microsoft Git's .github directory (or a subdirectory, if applicable). The following are notes on each of these files:

Love it! Great idea.

derrickstolee

Before I approve, do you have an example run of this action so I can see what the signed install flow looks like on macOS?

Edit: Of course, you already supplied a link. Thanks! https://github.com/ldennington/git/actions/runs/2580040902

.github/Makefile

.github/workflows/build-git-installers.yml

derrickstolee · 2022-06-29T13:36:51Z

Hm... I don't know how this happened:

$ git version
git version 2.32.1 (Apple Git-133)
$ which git
/usr/bin/git
$ /usr/local/bin/git
-bash: /usr/local/bin/git: Permission denied
$ ls -al /usr/local/bin/git
-rw-r--r--  1 root  wheel  4301248 Jun 28 21:13 /usr/local/bin/git

So... I've lost execute privileges on the Git binary! So the /usr/local/bin copy is skipped in favor of the Apple-installed one in /usr/bin.

ldennington · 2022-06-29T17:32:41Z

Hm... I don't know how this happened:
$ git version
git version 2.32.1 (Apple Git-133)
$ which git
/usr/bin/git
$ /usr/local/bin/git
-bash: /usr/local/bin/git: Permission denied
$ ls -al /usr/local/bin/git
-rw-r--r--  1 root  wheel  4301248 Jun 28 21:13 /usr/local/bin/git
So... I've lost execute privileges on the Git binary! So the /usr/local/bin copy is skipped in favor of the Apple-installed one in /usr/bin.

This was a great catch, thank you (and also a good reminder to me that simply making it through the install process is not enough)! I will investigate ASAP.

ldennington · 2022-07-14T21:57:14Z

@dscho @derrickstolee - I think we are good to go. Here is my latest successful run, and I'm currently running the version of microsoft/git I installed from the .dmg published by that run without issue.

Edit: Now we should be good - had to fix some whitespace and newline issues 😬.

derrickstolee

Looking good. Sorry to bother you about it, but could you rebase onto vfs-2.37.1?

derrickstolee · 2022-07-15T02:47:56Z

contrib/credential/osxkeychain/git-credential-osxkeychain.c

@@ -168,7 +168,7 @@ int main(int argc, const char **argv)
 		"usage: git credential-osxkeychain <get|store|erase>";

 	if (!argv[1])
-		die(usage);
+		die("%s", usage);


Are you working on an upstream fix for this? ;)

Yes! Today 😆.

dscho

Maybe we could add a step that mounts the .dmg, unpacks the .pkg and verifies that its git version works and shows the expected output? That would catch issues similar to the lost executable bit in the future.

dscho · 2022-06-29T14:22:25Z

.github/assets/etc/gitconfig.default

@@ -0,0 +1,58 @@
+[core]
+	excludesfile = ~/.gitignore
+	legacyheaders = false # >git 1.5


Do we have any code to handle core.legacyHeaders at all anymore? I do not see any, it seems that this has been removed in v1.5.3-rc0~218^2...

I actually removed this file based on your comment here.

So I don't think we'll need to worry about the legacy headers...apologies if I'm overlooking something though.

ldennington · 2022-07-15T15:37:48Z

Looking good. Sorry to bother you about it, but could you rebase onto vfs-2.37.1?

Will do - it's no problem.

ldennington · 2022-07-15T15:55:04Z

Maybe we could add a step that mounts the .dmg, unpacks the .pkg and verifies that its git version works and shows the expected output? That would catch issues similar to the lost executable bit in the future.

I think this is a good idea. However, it seems like we should add this type of validation for all the installers we produce. I've opened a separate work item for this: https://github.com/github/git-fundamentals/issues/965.

ldennington requested review from dscho and derrickstolee June 28, 2022 19:33

ldennington self-assigned this Jun 28, 2022

ldennington force-pushed the sign-macos branch from c623968 to b248e76 Compare June 28, 2022 19:47

ldennington requested a review from vdye June 28, 2022 21:52

ldennington force-pushed the sign-macos branch from b248e76 to 6b71c7d Compare June 29, 2022 01:52

derrickstolee reviewed Jun 29, 2022

View reviewed changes

ldennington force-pushed the sign-macos branch from 6b71c7d to c357441 Compare July 14, 2022 21:54

ldennington force-pushed the sign-macos branch 3 times, most recently from 7f458bd to 10600e3 Compare July 14, 2022 22:58

derrickstolee approved these changes Jul 15, 2022

View reviewed changes

dscho approved these changes Jul 15, 2022

View reviewed changes

fixup! contrib: add credential helper for OS X Keychain

5f3c042

ldennington changed the base branch from vfs-2.37.0 to vfs-2.37.1 July 15, 2022 15:57

ldennington force-pushed the sign-macos branch from 10600e3 to b4c150c Compare July 15, 2022 15:58

ldennington added 2 commits July 15, 2022 13:33

fixup! release: add Mac OSX installer build

225d94e

fixup! release: add signing step for .deb package

4c38f19

ldennington force-pushed the sign-macos branch from b4c150c to 4c38f19 Compare July 15, 2022 20:54

derrickstolee approved these changes Jul 18, 2022

View reviewed changes

ldennington merged commit c4a88d6 into microsoft:vfs-2.37.1 Jul 18, 2022

dscho mentioned this pull request Jan 25, 2023

Add a GitHub workflow definition timcharper/git_osx_installer#156

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sign and notarize macOS binaries #514

Sign and notarize macOS binaries #514

ldennington commented Jun 28, 2022 •

edited

Loading

derrickstolee commented Jun 29, 2022

derrickstolee left a comment •

edited

Loading

derrickstolee commented Jun 29, 2022

ldennington commented Jun 29, 2022

ldennington commented Jul 14, 2022 •

edited

Loading

derrickstolee left a comment

derrickstolee Jul 15, 2022

ldennington Jul 15, 2022

dscho left a comment

dscho Jun 29, 2022

ldennington Jul 15, 2022

ldennington Jul 15, 2022

ldennington commented Jul 15, 2022

ldennington commented Jul 15, 2022

Sign and notarize macOS binaries #514

Sign and notarize macOS binaries #514

Conversation

ldennington commented Jun 28, 2022 • edited Loading

derrickstolee commented Jun 29, 2022

derrickstolee left a comment • edited Loading

Choose a reason for hiding this comment

derrickstolee commented Jun 29, 2022

ldennington commented Jun 29, 2022

ldennington commented Jul 14, 2022 • edited Loading

derrickstolee left a comment

Choose a reason for hiding this comment

derrickstolee Jul 15, 2022

Choose a reason for hiding this comment

ldennington Jul 15, 2022

Choose a reason for hiding this comment

dscho left a comment

Choose a reason for hiding this comment

dscho Jun 29, 2022

Choose a reason for hiding this comment

ldennington Jul 15, 2022

Choose a reason for hiding this comment

ldennington Jul 15, 2022

Choose a reason for hiding this comment

ldennington commented Jul 15, 2022

ldennington commented Jul 15, 2022

ldennington commented Jun 28, 2022 •

edited

Loading

derrickstolee left a comment •

edited

Loading

ldennington commented Jul 14, 2022 •

edited

Loading