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

Work on CI code signing macOS .app #344

Conversation

xylix
Copy link
Contributor

@xylix xylix commented Feb 15, 2020

Changes were made mostly with these instructions . There's still a couple questionmarks, mostly about notarization and how the --deep flag is considered a bad practice and is there anything we could do with a pyinstaller .app about it to get rid of the need.

Also I am unsure how far using a self-signed unofficial cert will allow us to get, and at what point we will need the Apple Developer Program Id. At least before publishing in App store, for sure.

For these changes to properly work, we will need to add a certificate to the travis environment variables as a Base 64 string. The key for the cert (CERTIFICATE_OSX_P12) is defined in the new scripts/sign-macos.sh script, and the format has to be .p12. Also we will need to define a CERTIFICATE_PASSWORD env var to open the certificate securely.

I have a self-signed .p12 with a password set on my device, but I don't have travis access. @ErikBjare Any suggestions on how should I securely deliver the key or could you generate one on the mac you have access to and upload it to travis? (it's a relatively simple process, probably takes about 15-30 minutes.)

@xylix xylix changed the title WIP: Work on CI code signing macOS .app WIP (not merge ready): Work on CI code signing macOS .app Feb 15, 2020
@ErikBjare
Copy link
Member

the --deep flag is considered a bad practice and is there anything we could do with a pyinstaller .app about it to get rid of the need.

I don't know how codesign works for macOS, but since the suggested option is "just do what Xcode does automatically" I doubt we can do anything else. The process for us and Electron apps should be pretty similar, so we could always look at how they do things.

Also I am unsure how far using a self-signed unofficial cert will allow us to get, and at what point we will need the Apple Developer Program Id. At least before publishing in App store, for sure.

If getting an Apple Developer Program ID will get rid of the stupid warnings and get us a chance at getting ActivityWatch up on the App Store, I think we should just get one.

Any suggestions on how should I securely deliver the key or could you generate one on the mac you have access to and upload it to travis?

I can assign whatever secret environment variables that are needed to Travis, you just let me know which ones are needed :)

@ErikBjare
Copy link
Member

Keys have been set!

You might want to rebase on master once I've merged #347 which fixes CI.

@ErikBjare
Copy link
Member

Also, I found this by another open source project which uses PyInstaller and has jumped through the hoops of codesigning with PyInstaller: spesmilo/electrum#4994

@xylix
Copy link
Contributor Author

xylix commented Feb 16, 2020

Yeah I'll rebase after #347 .

Thanks for the link, looking into it.

@ErikBjare
Copy link
Member

ErikBjare commented Feb 16, 2020

I found this comment particularly interesting and relevant to our case: pyinstaller/pyinstaller#3991 (comment)

I should add that PyInstaller development is super slow due to funding issues, and I don't expect the PR I linked to get merged anytime soon.

@xylix
Copy link
Contributor Author

xylix commented Feb 16, 2020

Hm yes that comment does open up the problems and pyinstallers inner workings quite a bit more. The funding problems are quite sad :/

I'll do some testing with little snitch to confirm our signed .app has the same problems. Will also look into using xcode / some other apple tool for finding flaws with our signature method.

The pyinstaller 4.0 dev version we are using did have some improvements, but probably nothing related to the appsigning since that PR hasn't gotten merged.

@ErikBjare
Copy link
Member

ErikBjare commented Feb 16, 2020

If the PR not getting merged is an issue, we could always just fork PyInstaller and merge the needed changes ourselves.

@xylix
Copy link
Contributor Author

xylix commented Feb 17, 2020

True.

Now doing some research, I think what ccualianu is talking about in pyinstaller/pyinstaller#3991 (comment) : "When PyInstaller executes it writes to a tmp dir in $TMP/_MEIxxx. It extracts a bunch of the binaries packaged into your app there and then executes your app AGAIN from there." is only related to this setting https://pyinstaller.readthedocs.io/en/stable/usage.html#rarely-used-special-options or the other single-fileish option in pyinstaller, https://pyinstaller.readthedocs.io/en/stable/usage.html#using-upx .

Since we aren't using either, I don't think our pyinstaller executables are creating temporary environments that would require additional code signing work. On my computer I could confirm that aw-qt is indeed starting from within the .app directory, and not from some temp dir.

Screenshot 2020-02-17 at 10 15 22

So yeah, It seems like our signing should be fine for now altough the notarization is still a question mark. I'll rebase master now and work on getting CI signing working for this PR.

@xylix xylix added this to In progress in Road to 1.0 Feb 17, 2020
@xylix xylix self-assigned this Feb 17, 2020
@xylix xylix force-pushed the dev/sign-macos-travis branch 5 times, most recently from dd419b5 to 4ade2c0 Compare March 3, 2020 13:21
Fix travis warnings, cleanup is now disabled by default. Rename some keys which travis was aliasing anyway

Fixes for macOS signing

Add osx signing keys in before_install step
try fixing macos-signing script and makefile command. Add warning for running it locally

Move import-macos-cert script to scripts/ci

Increase macOS signing tool verbosity
@xylix xylix force-pushed the dev/sign-macos-travis branch 2 times, most recently from c4b328e to 717144d Compare March 3, 2020 18:00
@ErikBjare
Copy link
Member

ErikBjare commented Apr 1, 2020

If we want to properly notarize the app I found these which may be useful:

@@ -113,7 +114,7 @@ deploy:
- provider: releases
file_glob: true
file: dist/activitywatch-*.*
api_key:
token:
Copy link
Member

Choose a reason for hiding this comment

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

Why change this?

Copy link
Contributor Author

@xylix xylix Nov 23, 2020

Choose a reason for hiding this comment

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

IIRC this was just because the .travis linter was complaining about the older api_key key. Completely fine to leave this out from this PR

@ErikBjare ErikBjare marked this pull request as draft November 23, 2020 17:25
@ErikBjare ErikBjare changed the title WIP (not merge ready): Work on CI code signing macOS .app Work on CI code signing macOS .app Nov 23, 2020
@ErikBjare
Copy link
Member

ErikBjare commented Feb 1, 2021

I'm thinking through what would be needed to get this merged.

@ErikBjare
Copy link
Member

Merged in master, hoping for the best.

@ErikBjare ErikBjare marked this pull request as ready for review June 11, 2021 09:22
@ErikBjare ErikBjare added this to In progress in Research Ready Jun 11, 2021
@ErikBjare
Copy link
Member

Out of credits on Travis...

Apparently, macOS builds use up 5x the amount of credits compared to Linux builds:

image

@ErikBjare
Copy link
Member

ErikBjare commented Jun 11, 2021

@xylix Do you still have the CERTIFICATE_OSX_P12 and CERTIFICATE_PASSWORD?

I need to set them for GitHub Actions so we can move away from Travis. You can email them to me, encrypted with my PGP key.

@ErikBjare
Copy link
Member

@xylix I registered for an Apple Developer account myself (and paid the $99 yearly fee), so no need to find the keys anymore.

Once the application has processed, and I can codesign with my own keys, I'll try and get this PR merged.

@xylix
Copy link
Contributor Author

xylix commented Jun 16, 2021

Okay great! Sorry for the latency in responding.

I had the key files, but looking up how to make them into environment variables again takes some time. The article linked here https://github.com/ActivityWatch/activitywatch/pull/344/files#diff-af847b68547d3e6ddc481fb5b2c492cb112ea7245a5538d363db63ba3b6936a9R2 seems helpful.

@ErikBjare
Copy link
Member

Just realized this only codesigns the .dmg, not the .app within. Working on fixing it.

@ErikBjare ErikBjare added bounty An issue or PR with a bounty for solving platform: macos labels Aug 29, 2021
@ErikBjare ErikBjare changed the base branch from master to dev/macos-sign-and-notarize January 5, 2022 12:18
@ErikBjare
Copy link
Member

Merging this into the dev/macos-sign-and-notarize branch

@ErikBjare ErikBjare merged commit b515f96 into ActivityWatch:dev/macos-sign-and-notarize Jan 5, 2022
Road to 1.0 automation moved this from In progress to Done Jan 5, 2022
Research Ready automation moved this from In progress to Done Jan 5, 2022
@xylix xylix deleted the dev/sign-macos-travis branch January 5, 2022 14:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bounty An issue or PR with a bounty for solving platform: macos
Projects
Road to 1.0
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

2 participants