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

Feature: Sign macOS builds #8561

Merged
merged 1 commit into from Jan 13, 2021
Merged

Feature: Sign macOS builds #8561

merged 1 commit into from Jan 13, 2021

Conversation

orudge
Copy link
Contributor

@orudge orudge commented Jan 13, 2021

Fixes #7826.

Motivation / Problem

User has to jump through hoops to use a downloaded build on Intel Macs on recent versions of macOS.
User has to jump through even more hoops running Terminal commands to use a downloaded build on an Apple Silicon Mac.

Description

OpenTTD Distribution Ltd has applied for membership of the Apple Developer Program and now has a valid code signing certificate for distribution of Mac builds. This has been added into GitHub Secrets for OpenTTD and this pull request enables the creation of signed Mac builds. The builds are then notarized by Apple with the notarization ticket stapled to the .dmg. This prevents any GateKeeper prompts from appearing when the user has downloaded a build.

Limitations

Anybody who has forked the repository and runs the actions in their own branch will not be able to produce signed builds unless they provide their own certificate and developer ID. The builds can however be run successfully without signing or notarization.

@orudge orudge requested a review from TrueBrain January 13, 2021 12:00
Copy link
Member

@TrueBrain TrueBrain left a comment

Choose a reason for hiding this comment

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

Minor stuff; can't wait for this to be merged :D


dmg_filename=(bundles/*.dmg)

if [ "${dmg_filename}" = "bundles/*.dmg" ]; then
Copy link
Member

Choose a reason for hiding this comment

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

dmg_filename=$(ls bundles/*.dmg)

if [ -z "${dmg_filename}" ]; then ...

Might look a bit better.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That results in:

ls: bundles/*.dmg: No such file or directory

Taking out the set -e gives the same error followed by our custom error. Although it's not the most elegant, the code that's there seems to work without too much fuss.

Copy link
Member

@TrueBrain TrueBrain Jan 13, 2021

Choose a reason for hiding this comment

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

Oops:

$(ls bundles/*.dmg 2>/dev/null || true)

(it is about being explicit you expect failure, and handle it, basically :D)

Copy link
Contributor Author

@orudge orudge Jan 13, 2021

Choose a reason for hiding this comment

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

Nope, that doesn't then split the string up into an array if multiple DMGs happen to be in the folder. I'm sure there will be some further way around that, but it feels it's getting a bit more convoluted again. :)

(Though we do say to clear out the bundles directory first.)


if [ "${dmg_filename}" = "bundles/*.dmg" ]; then
echo No .dmg found in the bundles directory, skipping notarization. Please read this
echo script\'s source for execution instructions.
Copy link
Member

Choose a reason for hiding this comment

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

Please quote everything after echo. Also means you don't need to escape.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed (this did occur to me after I'd pushed!)

#!/bin/bash
set -e

# This script attempts to notarize the OpenTTD DMG generated by cpack.
Copy link
Member

Choose a reason for hiding this comment

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

nit: CPack

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

- name: Notarize
env:
AC_USERNAME: ${{ secrets.APPLE_DEVELOPER_USERNAME }}
AC_PASSWORD: ${{ secrets.APPLE_DEVELOPER_PASSWORD }}
Copy link
Member

Choose a reason for hiding this comment

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

I rather avoid people being confused that we user the developers username and password. You mention the username and password is app specific? Maybe better to name it as such? APPLE_APPLICATION_USERNAME ?

This just to avoid people misunderstanding what this is, and complaining about us not being secure or what-ever :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm, it's a username/password for the Apple Developer site, so it seemed a reasonable name to me. Can change if you would like but I don't think personally it's a cause for concern. I have mentioned in the comments in the .sh that it should ideally be an app-specific password that's used.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, but I know our audience a bit too well sadly :P

APPLE_DEVELOPER_SITE_USERNAME ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Have replaced with APPLE_DEVELOPER_APP_x, hopefully that's OK.

@orudge orudge merged commit 60851ef into OpenTTD:master Jan 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Mac OS binaries are unsigned
2 participants