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

[meta] GitHub Actions: Don't sign macOS builds from forked repo PRs #698

Merged
merged 3 commits into from
Aug 31, 2023

Conversation

confused-Techie
Copy link
Member

According to the docs a GitHub Action triggered from a forked repository, will be unable to access the secrets of the target repository, and instead will return an empty string, when asking for a secret.

What this means, is if a forked repo creates a PR to our repo, they will be unable to access the secrets of our repo, specifically the new macOS signing secrets. And instead the macOS signing secrets will be set with empty strings, which will cause Electron Builder to fail.

Resulting in logs like so:

empty password will be used for code signing  reason=CSC_KEY_PASSWORD is not defined

Which can be seen on #170 and #160

To avoid this, we will create a single step in GitHub Actions that can conditionally assign the env vars for macOS signing, only if the PR created stems from this exact repository.

It does mean that PRs generated from a forked repo will not be signed, and comes with the headaches involved there, but this is a necessary change if we want to achieve green CI, while still ensuring builds can be built on macOS properly.

@DeeDeeG
Copy link
Member

DeeDeeG commented Aug 31, 2023

Probably not necessary to consider this, since the above is probably going to work. But the other way I can think of is to unset the env vars if they're empty, just before running yarn dist, like so:

for i in CSC_LINK CSC_KEY_PASSWORD APPLEID APPLEID_PASSWORD TEAM_ID
do
  if [[ -z ${!i} ]]; then unset $i; fi
done
yarn dist

(This variable reading and unsetting logic is tested working locally on my macOS machine.)

That would build and sign if the env vars are really populated (non-fork runs), and build without signing on fork runs.

I am pretty neutral which approach is better, but I am leaning toward what is already committed just above. The way already committed only requires being able to read CI configs, but this requires some not trivial bash knowledge to read the workflow file, which is probably not worth it for best readability/maintainability for the most people.

@confused-Techie
Copy link
Member Author

Alright, @meadowsys & @DeeDeeG looks like these last changes worked how we expected.

image

You can see from our two choices, this PR was properly signed as we would expect

Copy link
Member

@DeeDeeG DeeDeeG left a comment

Choose a reason for hiding this comment

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

LGTM.

Comment might not be needed anymore, but I leave that to your judgment and would approve with or without the comment. 👍

Thanks for noticing this and addressing it!

Copy link
Member

@meadowsys meadowsys left a comment

Choose a reason for hiding this comment

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

looks good!

@confused-Techie
Copy link
Member Author

Thanks for both approvals! Modified the comment as requested, and I'll go ahead and merge!

@confused-Techie confused-Techie merged commit c96d60d into master Aug 31, 2023
@confused-Techie confused-Techie deleted the github-actions-dont-sign-forked-prs-macos branch August 31, 2023 03:13
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.

3 participants