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

Restore mac code-signing via our new code-signing service #1807

Open
wants to merge 33 commits into
base: main
Choose a base branch
from

Conversation

phargogh
Copy link
Member

@phargogh phargogh commented Feb 24, 2025

This PR restores mac DMG codesigning by way of our new codesigning service, which simplifies our actions YAML (since we now just build the workbench in all cases and then defer OS-specific codesigning logic to the codesigning service), and probably makes this whole process more secure by not copying certificates out of a GCS bucket and keeping the operational code on a better-protected host that we control.

NOTE: one of the puppeteer tests was reliably failing for me, co I temporarily commented out the puppeteer tests to make sure this codesigning functionality works as expected. You can see the passing mac and windows codesigning GHA steps here:

Fixes #1784

Checklist

  • Updated HISTORY.rst and link to any relevant issue (if these changes are user-facing)
    - [ ] Updated the user's guide (if needed)
    - [ ] Tested the Workbench UI (if relevant)

Copy link
Member

@dcdenu4 dcdenu4 left a comment

Choose a reason for hiding this comment

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

Thanks @phargogh for migrating this over to a unified code signing process. I only had one suggestion for updating a doc string.

@@ -0,0 +1,28 @@
#!/usr/bin/env sh
#
# Run this script to enqueue the windows binary for this current version of the
Copy link
Member

Choose a reason for hiding this comment

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

Update comment to mention mac now too or leave ambiguous?

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated in 3dd189d

@dcdenu4
Copy link
Member

dcdenu4 commented Feb 27, 2025

Hey @phargogh, I just thought about our makefile which still has codesign_mac and codesign_windows endpoints. At least for Windows this is no longer tenable, should we go ahead and remove them both from the makefile at this point?

@phargogh
Copy link
Member Author

Hey @phargogh, I just thought about our makefile which still has codesign_mac and codesign_windows endpoints. At least for Windows this is no longer tenable, should we go ahead and remove them both from the makefile at this point?

Yeah, I was kind of waffling on this a bit. From my perspective, the codesign_windows and codesign_mac steps are still correct processes (minus getting the certificate from GCS), so it wouldn't be incorrect to have the steps in there if we ever wanted to use them again. A case where we may want to use them again is if/when we need to manually sign an application.

Having said that, we're not actually using them, and everything related to the codesigning service is contained in the codesigning directory.

Anyways, I think I've talked myself into deleting these, so they have been removed/updated in 4de580b

@phargogh phargogh requested a review from dcdenu4 February 28, 2025 19:27
Copy link
Member

@dcdenu4 dcdenu4 left a comment

Choose a reason for hiding this comment

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

Thanks @phargogh for thinking about that and for the changes. This feels like the right direction to me and if we want to have a deliberate way to codesign manually in the future we can reestablish one based on the new structure here.

few other verbs described in the systemctl docs.
* `sudo journalctl -u natcap-codesign.service -n 300` will show the 300 most
recent log messages generated by the service.
* `sudo journalctl -u natcap-codesign.service -n 300 --follow` will show the
Copy link
Member

Choose a reason for hiding this comment

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

Very helpful to have this documented here!

url_base=$(make -C .. --no-print-directory print-DIST_URL_BASE | awk ' { print $3 } ')
platform=$(python -c "import platform;p=platform.system().lower();print(p if p != 'windows' else 'win32')")

if [ "$platform" = "win32" ]; then
Copy link
Member

Choose a reason for hiding this comment

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

Would it be worth scripting this in python? Since there's already some python commands in here, and the other codesigning scripts are in python.

Copy link
Member

@emlys emlys left a comment

Choose a reason for hiding this comment

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

Thanks @phargogh! I am approving and I had just one suggestion about a script

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.

Renew our MacOS Code-Signing Certificate
3 participants