Automate signed pkg build for macOS App Store submission #2624
Conversation
.github/autobuild/mac.sh
Outdated
| # MACOS_CERTIFICATE - Mac Installer Distribution | ||
| # MACAPP_CERTIFICATE - Mac App Distribution |
There was a problem hiding this comment.
I think you should put this comment a few lines above to show the difference between MACOS_CERTIFICATE and MACAPP_CERTIFICATE.
Honestly, I'd prefer something clearer (something with deployment or distribution)
There was a problem hiding this comment.
Thanks for picking up on this :)
I didn't want to break the existing MACOS_CERTIFICATE but ideally I agree, it should be something like:
MAC_INST_DIST_CERT - for Mac Installer Distribution certificate (for signing the pkg file)
MAC_APP_DIST_CERT - for Mac App Distribution certificate (for codesigning - both adhoc and app-store releases)
and make the Github Secrets names correspond appropriately.
You also need to store a password and the ID for each certificate (though you can re-use the same password for both certs)
How it could possibly look in autobuild.yml:
env:
JAMULUS_BUILD_VERSION: ${{ needs.create_release.outputs.build_version }}
MAC_INST_DIST_CERT: ${{ secrets.MAC_INST_DIST_CERT}}
MAC_INST_DIST_CERT_PWD: ${{ secrets.MAC_INST_DIST_CERT_PWD }}
MAC_INST_DIST_CERT_ID: ${{ secrets.MAC_INST_DIST_CERT_ID }}
MAC_APP_DIST_CERT: ${{ secrets.MAC_APP_DIST_CERT}}
MAC_APP_DIST_CERT_PWD: ${{ secrets.MAC_APP_DIST_CERT_PWD }}
MAC_APP_DIST_CERT_ID: ${{ secrets.MAC_APP_DIST_CERT_ID }}
.github/autobuild/mac.sh
Outdated
| security set-key-partition-list -S apple-tool:,apple:,codesign: -s -k "${KEYCHAIN_PASSWORD}" build.keychain | ||
| security import macinst_certificate.p12 -k build.keychain -P "${MACOS_CERTIFICATE_PWD}" -A -T /usr/bin/productbuild | ||
| security import macapp_certificate.p12 -k build.keychain -P "${MACAPP_CERTIFICATE_PWD}" -A -T /usr/bin/codesign | ||
| # security set-key-partition-list -S apple-tool:,apple:,codesign: -s -k "${KEYCHAIN_PASSWORD}" build.keychain |
There was a problem hiding this comment.
Please try to avoid commented code.
There was a problem hiding this comment.
This is cruft from testing :)
Will remove
.github/autobuild/mac.sh
Outdated
| security set-key-partition-list -S apple-tool:,apple: -s -k "${KEYCHAIN_PASSWORD}" build.keychain | ||
|
|
||
| # set lock timeout on keychain to 6 hours | ||
| security set-keychain-settings -lut 21600 |
There was a problem hiding this comment.
Why 6 h? Probably due to automatic lock and a resulting failure. Is there a cleaner way? Why is this needed now and wasn't previously?
There was a problem hiding this comment.
Good question. One of the hardest parts in getting this to work was working out how and why either codesign or productbuild would hang, likely due to prompting interactively for authentication.
Apple does a terrible job at documenting the necessary arguments for:
security set-key-partition-list -S ...
and the effectiveness of the -A and -T options on security import had to be worked out through trial and error.
It's possible that this extended timeout is redundant, but it is apparently a reasonable setting for similar project builds, especially as these are being run on ephemeral hosted Github Action runners.
There was a problem hiding this comment.
I think this part (line 62 & 63) is redundant by now as we have removed the re-locking timeout altogether: https://github.com/jamulussoftware/jamulus/pull/2927/files
(Haven't looked at the other parts of the PR yet)
There was a problem hiding this comment.
I'd like to test if the CodeQl issue is fixed with this instead.
There was a problem hiding this comment.
I guess it's worth a test, but I wouldn't expect too much based on the docs and comments elsewhere:
https://ss64.com/osx/security-keychain-settings.html
https://developer.apple.com/forums/thread/690665
There was a problem hiding this comment.
removed the redundant timeout setting
.github/autobuild/mac.sh
Outdated
| # for debug | ||
| echo "Checking found identities..." | ||
| security find-identity -v |
There was a problem hiding this comment.
This could for example be commented or removed entirely if it's only for debugging.
There was a problem hiding this comment.
Yes I'll remove this
mac/deploy_mac.sh
Outdated
| fi | ||
| mv "${build_path}/${target_name}.app" "${deploy_path}" | ||
|
|
||
| # move things |
There was a problem hiding this comment.
Which things?You mean dmg file or pkg file?
There was a problem hiding this comment.
I'll clarify the comments here
There was a problem hiding this comment.
I think we need to make this work along side the ad-hoc signing.
The changes here will break the existing signing set up.
Ad hoc requires:
- Signing
- Notarizing
- Stapling the package when notarization complete
App Store distribution requires
- Signing (with a different certificate)
- Packaging
- Installer signing (with an installer certificate)
- Validation
- Upload
I'd like to see the App Store target work along side the existing ad-hoc signing steps.
.github/autobuild/mac.sh
Outdated
| @@ -30,20 +30,35 @@ prepare_signing() { | |||
| [[ -n "${MACOS_CERTIFICATE:-}" ]] || return 1 | |||
| [[ -n "${MACOS_CERTIFICATE_ID:-}" ]] || return 1 | |||
| [[ -n "${MACOS_CERTIFICATE_PWD:-}" ]] || return 1 | |||
| [[ -n "${MACAPP_CERTIFICATE:-}" ]] || return 1 | |||
There was a problem hiding this comment.
This needs to be modified a bit.
I don't want the existing signing mechanism to be broken, which is what the MACOS_CERTIFICATE stuff is all about.
The App Store version needs a different signing and installer signing certificates and these need to be broken out.
.github/autobuild/mac.sh
Outdated
| ## Put the certs to a file | ||
| # MACOS_CERTIFICATE - Mac Installer Distribution | ||
| # MACAPP_CERTIFICATE - Mac App Distribution | ||
| echo "${MACOS_CERTIFICATE}" | base64 --decode > macinst_certificate.p12 |
There was a problem hiding this comment.
It looks like you're repurposing the existing signing cert for the App Store - this will break the non-app store signing.
Can you separate out the new requisite certs and name the env vars appropriately?
mac/deploy_mac.sh
Outdated
| macdeployqt "${build_path}/${target_name}.app" -verbose=2 -always-overwrite | ||
| else | ||
| macdeployqt "${build_path}/${target_name}.app" -verbose=2 -always-overwrite -hardened-runtime -timestamp -appstore-compliant -sign-for-notarization="${cert_name}" | ||
| macdeployqt "${build_path}/${target_name}.app" -verbose=2 -always-overwrite -hardened-runtime -timestamp -appstore-compliant -sign-for-notarization="${macapp_cert_name}" |
There was a problem hiding this comment.
All of this will break the existing ad-hoc signing and notarizing.
I think we need a second step here that signs a separate artifact for the Mac app store.
There was a problem hiding this comment.
Ok, I think I snarled things up a bit by renaming things unnecessarily.
Currently my build process produces both pkg and dmg files, and I assume the dmg file will work fine with the notarization process for adhoc release (I haven't tested that obviously as notarization is only for non app store distribution).
As far as I understand it, the certificate that you are currently using for codesigning for adhoc release can be used also for the codesigning for app-store release.
App Store distribution just requires the additional 1 certificate for use by productbuild - described as following in Apple Developer web UI:
Mac Installer Distribution
This certificate is used to sign your app's Installer Package for submission to the Mac App Store.
So: let me fix the cert and variable renaming so it's a transparent change, and I'll look at automating the validate and upload stages with xcrun altool as I mentioned in the iOS PR.
There was a problem hiding this comment.
Ok scratch that :) you are probably using the following cert type for your current code-signing:
Developer ID Application
This certificate is used to code sign your app for distribution outside of the Mac App Store.
In which case indeed we do need a separate certificate for codesigning for App Store release.
Ok so I will rework by adding 2 x additional certs in parallel, to not disturb the current adhoc release workflow.
|
Good to hear! |
Yes sure, and from the build checks above it looks like it works transparently when signing deps are not satisfied. As I noted in the iOS PR #2625 the build now attempts to validate and upload to App Store using |
|
@emlynmac any news? |
danryu
changed the title
|
Updated with the necessary logic to validate and upload the signed macOS "pkg" installer to the Mac App Store when the necessary conditions are met (I thought I had already done this!) |
.github/autobuild/mac.sh
Outdated
| if [ -f ./deploy/Jamulus_*.pkg ]; then | ||
| echo "Moving build artifact2 to deploy/${artifact2}" | ||
| mv ./deploy/Jamulus_*.pkg "./deploy/${artifact2}" | ||
| echo "::set-output name=artifact_2::${artifact2}" |
There was a problem hiding this comment.
As GitHub is deprecating set-output, Please use environment files.
.github/autobuild/mac.sh
Outdated
| # test the signature of package | ||
| pkgutil --check-signature "${ARTIFACT_PATH}" | ||
| # attempt validate and then upload of pkg file, using previously-made keychain item | ||
| xcrun altool --validate-app -f "${ARTIFACT_PATH}" -t macos -u $NOTARIZATION_USERNAME -p $NOTARIZATION_PASSWORD |
There was a problem hiding this comment.
Not sure about the quoting of the Password here.
There was a problem hiding this comment.
Values are output as ***, nothing sensitive is written to log
.github/autobuild/mac.sh
Outdated
| xcrun altool --validate-app -f "${ARTIFACT_PATH}" -t macos -u $NOTARIZATION_USERNAME -p $NOTARIZATION_PASSWORD | ||
| xcrun altool --upload-app -f "${ARTIFACT_PATH}" -t macos -u $NOTARIZATION_USERNAME -p $NOTARIZATION_PASSWORD | ||
|
|
||
| ## altool is deprecated - migrate soon to using notarytool eg: |
There was a problem hiding this comment.
It can, sure. But altool will work for another 12 months, and I am loathe to change the CI when it is working (as it is for me).
So I put a helpful comment for the near future. It requires some more keychain manipulation so it's not just a drop-in replacement.
There was a problem hiding this comment.
Hmm. Still shouldn't we use the more future proof way as soon as possible (prevents duplicate work and debugging if it's suddenly gone).
There was a problem hiding this comment.
In general, I agree, I'm just tight on time. I'll take a look today if I can
There was a problem hiding this comment.
Updated, now working with notarytool. It was easier than expected - uses the same credentials as in Notarization.
Note: the Github Action (https://github.com/devbotsxyz/xcode-notarize) used currently for dmg Notarization still uses the deprecated altool in the background. And that repo hasn't been updated in 2 years :/
.github/workflows/autobuild.yml
Outdated
| @@ -260,6 +266,7 @@ jobs: | |||
| retention-days: 31 | |||
| if-no-files-found: error | |||
|
|
|||
| #NOTE: not doing auto notarize/staple for now | |||
There was a problem hiding this comment.
crufty comment, removed
mac/deploy_mac.sh
Outdated
| @@ -47,6 +68,9 @@ build_app() | |||
| local job_count | |||
| job_count=$(sysctl -n hw.ncpu) | |||
|
|
|||
| # Get Jamulus version | |||
| local app_version="$(cat "${project_path}" | sed -nE 's/^VERSION *= *(.*)$/\1/p')" | |||
There was a problem hiding this comment.
Don't we have en environment variable for the version? Preferably it should be considered in all scripts.
There was a problem hiding this comment.
Good point. I was copying a previous pattern from deploy_mac. Now updated to use JAMULUS_BUILD_VERSION
The cert stuff is obviously most easily handled by whoever already has the App Store Connect account for Jamulus. I don't think the certs are any more sensitive than the existing auth details you are already using for dmg file Notarization ( |
|
Fair point. Emlyn owns the cert for now and not many people have push access to his repo. |
|
When/if he gets round to it Emlyn will have to create the 2 new certificates and follow the guides as mentioned in the description. Apple doesn't make this easy enough IMO - too much downloading certificates, importing to keychain manager, exporting as p12 then base64 encoding - for each cert. It's a pain. |
|
Coding style checks now fixed |
|
Great! Thanks. Maybe this gets ready for 3.11.0 (next release, not this one) |
|
@pljones Should this be assigned to me currently? It needs another review... |
|
Just as quick unrelated comment: The notarization/part of signing action seems to be no longer available. |
The "Reviewers" listed should be reviewing. If they raise any comments, it's still with you to take it forwards. |
|
@danryu I believe the comment by emlynmac is still outstanding |
Addressed in May '22 |
Yup, I know how it works. Hence why I asked why it's assigned to me, as I've taken it forwards on each comment raised ... |
|
Uh sorry. Probably it needs testing on a repo with a cert then. |
|
@danryu do you have signing set up in your repo? Maybe you can test it yourself? |
|
I'll close this PR if it doesn't get any progress in the next month. The build is broken. |
That would be a shame.
That has nothing to do with this PR. |
|
The question is: is it mergable? |
With a little effort due to the inevitable drift, yes. |
|
Yeah. We have lost signing a while ago, so it would be great if you handle that. |
|
Concerning notarization: can we switch to a local notarization and get rid of the action we use currently? Probably yes, I suppose. I'd like to get rid of the external notarization as it opens another potential source we cannot control. |
|
Notarization can be done with Xcode on macOS, but it still requires having a paid Apple Developer account and contacting Apple servers for the actual notarization process. There would be no gain in removing the CI for notarization. To be clear, this PR was created to automate the process not just of notarizing the macOS Jamulus package (which was already implemented) but of completing the whole process necessary to submit the package to the macOS App Store. This process is somewhat more involved. Regarding the current state of the Jamulus macOS build (and the lack of notarization support), that is a separate issue and should probably not be tracked in this thread. PS What happened to the previous macOS maintainer (who clearly had a paid macOS developer account)? |
We don't know for sure TBH. But he told us that he won't be reliably available to sign. |
Yes. But it's an issue which will most likely come up if we trigger a rebuild. |
|
Ok, I need to check things on Apple Developer again (it's been a while), but as long as there's no significant overhead in terms of certificate management etc, I should be able to host the Jamulus macOS build and get that part unblocked. I just need a little more time to finish my current project before I can attend to this properly, hopefully next week. |
|
Hi, is this still something that can get done shortly and will be sustainable for further releases? |
|
I believe so. |
This PR adds automation to create a signed pkg (installer) file for direct submission to the macOS App Store.
CHANGELOG: adds macOS signed pkg build automation
Context: automates building of signed pkg file for macOS App Store
Does this change need documentation? What needs to be documented and how?
Required:
Certificates:
Identifier:
Status of this Pull Request
What is missing until this pull request can be merged?
Checklist