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

Quicksilver is not codesigned #2654

Closed
pjrobertson opened this issue Feb 23, 2022 · 19 comments · Fixed by #2655
Closed

Quicksilver is not codesigned #2654

pjrobertson opened this issue Feb 23, 2022 · 19 comments · Fixed by #2655
Milestone

Comments

@pjrobertson
Copy link
Member

For previous discussions, see #2555

Quicksilver is currently not codesigned on release, causing issues with users opening it on their computer.

Attempts were made to fix codesigning in #2647, however those changes only codesign the app and not the DMG. Currently outstanding is codesigning the DMG.

This is the main blocking issue before we can get 2.0 out.

@pjrobertson pjrobertson added this to the 2.0.0 milestone Feb 23, 2022
@n8henrie
Copy link
Member

I'd like to continue working on this.

$ xcrun notarytool submit Quicksilver\ 1.6.1.dmg --wait --team-id '...' --apple-id '...' --password '...'
Conducting pre-submission checks for Quicksilver 1.6.1.dmg and initiating connection to the Apple notary service...
Error: HTTP status code: 403. Invalid or inaccessible developer team ID for the provided Apple ID. Ensure the Team ID is correct and that you are a member of that team.

@skurfer would you add me to your Apple "team," which should allow me to sign using my own ID and password?

Based on https://help.apple.com/developer-account/#/dev3e8818774 it should be:

https://developer.apple.com/account -> People -> Manage Users; my name and Apple ID from email address we've used for correspondence.

@n8henrie
Copy link
Member

Perhaps a handy Apple-official link to refer to in documentation: https://support.apple.com/en-us/HT202491

@skurfer
Copy link
Member

skurfer commented Feb 26, 2022

I don’t see “People”. Maybe because I have an individual membership and not a business?

@n8henrie
Copy link
Member

Ah, that makes sense.

If you’re enrolled in the Apple Developer Enterprise Program...

@n8henrie
Copy link
Member

n8henrie commented Mar 1, 2022

WOOHOO! Got it.

@n8henrie
Copy link
Member

n8henrie commented Mar 1, 2022

What a relief.

@skurfer @pjrobertson can you guys take a look at https://github.com/quicksilver/Quicksilver/actions/runs/1913370728?

I think the hard part (of signing) is done, but I'm sure there are a few more tweaks before merging. Specifically, I'm going to do a little tinkering tomorrow with (fake) repo secrets and convince myself of the security here. Again, I think limiting the qssign step to tags instead of all pull requests should go a long way here.

Also, I'd like some feedback on #2655 (comment) -- specifically I think we'll probably want to continue the current logic for naming the DMG (the version part), but I think we'll likely need to manually keep track of the git tag to have it properly named in the file uploaded to releases. Will tinker with this as well.

Good progress though, I'm psyched.

@skurfer
Copy link
Member

skurfer commented Mar 1, 2022

Whoa! Notarization works and the DMG is accepted. Nice work! (I tried building and notarizing locally as well and got the same result.)

What’s with the notarytool crash? Was it on one of the status checks? Because the process obviously worked.

One small thing: My keychain profile is called “Quicksilver Notarization”. Can you make that an environment variable that just defaults to “Quicksilver” instead of hard-coding it?

As an alternative, we could take 1.6.1 from the tag, which would give us some flexibility to e.g.: git tag 1.6.1-hotfix && git push --tags -> Quicksilver 1.6.1-hotfix.dmg

What’s the use case? Would we ever want to release something with a version number that looked like that? Or would that only affect the DMG filename and not the app’s version?

FYI, the version ultimately comes from QS_INFO_VERSION in Configuration/Developer, not Info.plist.

@pjrobertson
Copy link
Member Author

pjrobertson commented Mar 1, 2022 via email

@n8henrie
Copy link
Member

n8henrie commented Mar 1, 2022

Security:

Their overview is pretty good: https://docs.github.com/en/actions/security-guides/security-hardening-for-github-actions

It covers topics like code injection via maliciously crafted PR titles (e.g. pull request title a"; ls $GITHUB_WORKSPACE"), masking secret values in logs, and requiring specific privileges for PRs that modify files in github/workflows via CODEOWNERS:

https://docs.github.com/en/repositories/managing-your-repositorys-settings-and-features/customizing-your-repository/about-code-owners

  • I added a CODEOWNERS file to /.github to protect workflows from alteration
  • I added a fake secret (now deleted) and made a few PR pushes to see if I could leak it
  • It seems difficult to leak secrets in the logs visible in the WebUI:
    • These are all masked:

      • echo "${{ secrets.QS_FAKE_SECRET }}"
      • echo -n "${{ secrets.QS_FAKE_SECRET }}"
      • QS_SECRET="${{ secrets.QS_FAKE_SECRET }}"; echo "$QS_SECRET"
      • QS_SECRET="${{ secrets.QS_FAKE_SECRET }}"; bash -c 'echo "$QS_SECRET"'
  • It is unsurprisingly not difficult to leak in artifacts:
    • echo "${{ secrets.QS_FAKE_SECRET }}" > my_secret
    • add an action to persist artifact my_secret
    • download artifact, contains secret in plain text

I also used a separate account that I have, created a new repo, and then issues a PR from my main account that added a file to .github/workflows/ in order to simulate someone creating a new action / see what would happen in the situation of a malicious PR.

It does note on the page for managing secrets:

Secrets are environment variables that are encrypted. Anyone with collaborator access to this repository can use these secrets for Actions.

Secrets are not passed to workflows that are triggered by a pull request from a fork.

In keeping with this, in my testing, the secret was blank when the action was run from a PR sent in by a user that did not have write access to the test repository.

So I think this all seems pretty good.

@n8henrie
Copy link
Member

n8henrie commented Mar 1, 2022

@skurfer

What’s with the notarytool crash?

I have no idea, wasn't really sure how to debug it either.

My keychain profile is called “Quicksilver Notarization”. Can you make that an environment variable that just defaults to “Quicksilver” instead of hard-coding it?

How about I just make it default to Quicksilver Notarization? It might as well default to yours, since none of us have the setup for notarization (you're the only one that should be able to successfully run qssign locally).

dc6cb77dab02e328b0fc4152d868a989417a38ac

What’s the use case? Would we ever want to release something with a version number that looked like that?

For a beta release, for example.

Or would that only affect the DMG filename and not the app’s version?

I would think both -- continuing the example of a beta release, the DMG to make it obvious when a user goes through their downloads folder a month later that this was Quicksilver 2.0-beta.dmg, and in the app version for use in submitting logs or crash reports.

FYI, the version ultimately comes from QS_INFO_VERSION in Configuration/Developer, not Info.plist.

Right, gotcha.

I don't think the tag naming thing is particularly important, I'm fine going without if there's not a terrible lot of enthusiasm. We can always revisit later, it shouldn't hold us back.

@n8henrie
Copy link
Member

n8henrie commented Mar 1, 2022

As for triggering release builds, tags could work, but I still think simply pushing a commit “RELEASE” to master is simple enough. Some other thoughts in favour of this:

  • I think it’s more readable to those visiting the repo - they can see the release info right in the commit history
  • I’m not a big fan of how Github displays tags, as I think it can be confusing for users: e.g. I go to download what looks like version 1.6.1 (v1.6.1.tar.gz) of Quicksilver and it ends up being a load of jumbled code.
  • As Rob pointed out - the version number doesn’t come from the tag itself, but from the QS_INFO_VERSION val - so by just pushing a commit to master, we wouldn’t need to actually create anything new.

To clarify -- the release itself will actually go to the Releases tab. The action by default uses the tag name as the release name; I'm not sure (though I can try to figure out) how to run arbitrary code in the Release action to scrape the name from e.g. xcodebuild -showBuildSettings.

I would also say we stay away from the idea of non-conforming naming/version numbers: e.g. ‘1.6.1-hotfix’ - I think that may just cause headaches down the line.

No problem, we can revisit if we want to consider for beta testing at some point.

There are multiple artefacts here: https://github.com/quicksilver/Quicksilver/actions/runs/1913370728 Ideally there’d just be the DMG

I'm not sure I agree -- the way I see it, the build artifacts serve a few purposes:

  • to pass the raw materials from the build action to the sign action (DMG_INGREDIENTS)
  • to allow us to debug build issues and behavior without necessarily having to build locally
    • this is why I included the build settings there, though on second though I think I'll move this into DMG_INGREDIENTS and just move it back out before building the DMG from the directory
    • Example: user reports an issue. We can make a change, push to a branch, have the user download and run the build qs.app artifact to see if the issue is resolved.

Additionally, IMO we do not generally want to build the full codesigned dmg for all PRs -- in part because these are all officially signed. In my mind, it would make sense to only really sign a release-quality product. Perhaps I'm being too cautious here, but I wouldn't want some incompletely considered code from a feature branch be in the wild with an official signature on it. As a side-note, I'd previously had some concerns about users creating a malicious PR that could get signed (and could jeopardize the legitimacy of the cert if Apple caught wind), but it looks like secrets are empty for non-contributor PRs, so it should just fail.

So I think the tags and build artifacts solve a few issues in a pretty easy way and have a few advantages:

  • I can limit the sign action to run only on a new tag, so code that is "not quite ready" doesn't carry an official signature
  • tags can be GPG signed
  • a tag provide a really easy way to provide the release name
  • I think of tags as the de-facto way to check out a release build from a repo; git checkout "$(git tag --sort=creatordate | tail -n 1)" is a pretty common move when I'm building from source.

I see there are a load of tags like 0.0.1 and 0.0.2 from last November. Looks like they were created by Nate. Can those be deleted?

Had forgotten about those. I'll take a look.

@n8henrie
Copy link
Member

n8henrie commented Mar 1, 2022

I see there are a load of tags like 0.0.1 and 0.0.2 from last November. Looks like they were created by Nate. Can those be deleted?

Done

mapfile -t tags < <(git tag --sort=creatordate --format='%(authorname) %(refname:lstrip=2)' |
  awk '/^Nathan Henrie/ { print $NF }')

for tag in "${tags[@]}"; do
  git tag --delete "${tag}"
  git push origin :refs/tags/"${tag}"
done

https://github.com/quicksilver/Quicksilver/tags

Most recent is now https://github.com/quicksilver/Quicksilver/releases/tag/v1.6.1

@n8henrie
Copy link
Member

n8henrie commented Mar 1, 2022

I've since deleted it, but here is what appears on https://github.com/quicksilver/Quicksilver/releases when I run:

git tag -s test-tag-release-2

Screen Shot 2022-03-01 at 16 35 33

As you can see it's successfully pulling the release name from QS_INFO_VERSION irrespective of the tag.

I'm not sure why it replaced the space with a . in between Quicksilver and 1.6.1. The logs make it look like it should have a space:

2022-03-01T22:25:17.5376590Z ##[group]Run softprops/action-gh-release@v1
2022-03-01T22:25:17.5377100Z with:
2022-03-01T22:25:17.5377440Z   files: /tmp/QS/build/Release/Quicksilver*.dmg
2022-03-01T22:25:17.5377890Z   token: ***
2022-03-01T22:25:17.5378160Z env:
2022-03-01T22:25:17.5393060Z   MACOS_CERTIFICATE: ***
2022-03-01T22:25:17.5393440Z   MACOS_CERTIFICATE_PASSWORD: ***
2022-03-01T22:25:17.5393830Z   KEYCHAIN_PASSWORD: ***
2022-03-01T22:25:17.5394170Z   SIGNING_IDENTITY: ***
2022-03-01T22:25:17.5394640Z   NOTARIZING_ID: ***
2022-03-01T22:25:17.5394990Z   NOTARIZING_PASS: ***
2022-03-01T22:25:17.5395340Z   KEYCHAIN_PROFILE: Quicksilver Notarization
2022-03-01T22:25:17.5395680Z   QS_INFO_VERSION: 1.6.1
2022-03-01T22:25:17.5396180Z ##[endgroup]
2022-03-01T22:25:17.8178470Z 👩‍🏭 Creating new GitHub release for tag test-tag-release-2...
2022-03-01T22:25:18.2136360Z ⬆️ Uploading Quicksilver 1.6.1.dmg...

@pjrobertson I think this is pretty unambiguous (and comparable to many other large projects) with respect to the artifacts available on the releases page -- just the DMG and the source code (as compared to short-lived artifacts that are available for each run).

@n8henrie
Copy link
Member

n8henrie commented Mar 1, 2022

Re: spaces in filename, I bet it's this, which references https://docs.github.com/en/rest/reference/releases#upload-a-release-asset:

GitHub renames asset filenames that have special characters, non-alphanumeric characters, and leading or trailing periods.

@n8henrie
Copy link
Member

n8henrie commented Mar 2, 2022

Oh, and just double checking from the downloaded dmg:

$ file Contents/MacOS/Quicksilver 
Contents/MacOS/Quicksilver: Mach-O universal binary with 2 architectures: [x86_64:Mach-O 64-bit executable x86_64] [arm64:Mach-O 64-bit executable arm64]
Contents/MacOS/Quicksilver (for architecture x86_64):	Mach-O 64-bit executable x86_64
Contents/MacOS/Quicksilver (for architecture arm64):	Mach-O 64-bit executable arm64
$ file ./Contents/Resources/QSDroplet.app/Contents/MacOS/QSDroplet
./Contents/Resources/QSDroplet.app/Contents/MacOS/QSDroplet: Mach-O universal binary with 2 architectures: [x86_64:Mach-O 64-bit executable x86_64] [arm64:Mach-O 64-bit executable arm64]
./Contents/Resources/QSDroplet.app/Contents/MacOS/QSDroplet (for architecture x86_64):	Mach-O 64-bit executable x86_64
./Contents/Resources/QSDroplet.app/Contents/MacOS/QSDroplet (for architecture arm64):	Mach-O 64-bit executable arm64

@pjrobertson can make arm64 builds now :)

I'm still working on a couple minor issues:

  • can we get rid of the ugly but perhaps unimportant NSFileHandleOperationException Broken pipe error?
  • Do we need to --force codesigning? Answer: yup

@pjrobertson
Copy link
Member Author

Looks good!

Releases with source code + a DMG are good, as long as the source code files are named descriptively (Source Code (zip) and Source Code (tar.gz) work.

In my mind, it would make sense to only really sign a release-quality product.

Agreed. Signed release only on a new tag or a special commit to master. Looks like you've gone with tag - looks good.

To clarify -- the release itself will actually go to the Releases tab.

OK, that's good. I like that

(In reply to 'there should only be a DMG in artefacts): I'm not sure I agree -- the way I see it, the build artifacts serve a few purposes...

OK, no problem then. If we're not expecting average users to go find artefacts, and we have a 'Release', then I'm fine with this.

@pjrobertson can make arm64 builds now :)

For Quicksilver, but not plugins I guess ;-) hahaha. Jokes aside: I think we should stick to one person in charge of the release process. Historically it's been Rob.

With that said, there are a few steps in the release process page which are still manual (maybe that's best for now):

  • upload to the website with qs-push-plugin
  • Pull the latest transifex changes
  • Update gh-pages so this page gets updated
  • Update download.php on qsapp.com

One other point:

  • Can we rename ci.yml (and the name: build to something a little more descriptive?)

@n8henrie
Copy link
Member

n8henrie commented Mar 3, 2022

For Quicksilver, but not plugins I guess

I will probably create a generic GitHub Actions workflow for plugins, perhaps then :)

... which are still manual (maybe that's best for now):

👍

Pull the latest transifex changes

I don't know what a transifex is (yet), but looks like https://www.transifex.net/projects/p/quicksilver/ is down.

$ dig +short 'transifex.net'
$

https://downforeveryoneorjustme.com/transifex.net?proto=https&www=1

Can we rename ci.yml (and the name: build to something a little more descriptive?)

Creativity is often not my strong suit. How about github-actions.yml and name: xcodebuild? I'm open to suggestions here :)

@n8henrie
Copy link
Member

n8henrie commented Mar 3, 2022

What’s with the notarytool crash? Was it on one of the status checks?

@skurfer Broken pipe error is resolved, seems to have been due to notarytool ... | awk '{ print $2; exit }', where the early exit was because the submission ID appears multiple times, but clearly notarytool didn't like being hung up on. Changed to just overwrite a variable id and only print it once (at the END).

n8henrie added a commit that referenced this issue Mar 3, 2022
Fixes #2654

Issues addressed:

- Provides separate GitHub Actions for building and signing (#2583 (comment))
- Provides separate script for debugging signing so you don't have to rebuild every time (requires exporting a few variables normally set in `qsrelease`
- By default will still build *and* sign (for local builds) unless `QS_BUILD_ONLY` is set -- preserves current behavior
- Uses GitHub Actions' artifacts to avoid re-building the entire project twice
- Removes the "arbitrary volume size and hope it's big enough" workaround
- Adds what I think should be the necessary changes for automatic
  notarization of the DMG

Other changes:

- Removes need for `buildDMG.pl` with no new dependencies
- Reorders test *after* build, since the tests depend on `/tmp/QS/Configuration/Quicksilver.pch`
- Split uploads into separate named actions
- Copy the codesigned app to parent directory for easy acess
- Create a zip of QS.app as a convenience build artifact
- Specify release config for testing
- Use `working-directory` instead of `cd` for several actions
- Rename `release.yaml` to `ci.yaml` as it now has separate stages for
  build, sign, and release
@n8henrie
Copy link
Member

n8henrie commented Mar 3, 2022

So I think all my major issues and concerns have been addressed at this point. I've rebased, squashed, and force-pushed, so I think #2655 is ready to merge, which should close this issue.

I'm happy to make more changes if desired (like renaming things) -- just let me know!

skurfer pushed a commit that referenced this issue Mar 3, 2022
Fixes #2654

Issues addressed:

- Provides separate GitHub Actions for building and signing (#2583 (comment))
- Provides separate script for debugging signing so you don't have to rebuild every time (requires exporting a few variables normally set in `qsrelease`
- By default will still build *and* sign (for local builds) unless `QS_BUILD_ONLY` is set -- preserves current behavior
- Uses GitHub Actions' artifacts to avoid re-building the entire project twice
- Removes the "arbitrary volume size and hope it's big enough" workaround
- Adds what I think should be the necessary changes for automatic
  notarization of the DMG

Other changes:

- Removes need for `buildDMG.pl` with no new dependencies
- Reorders test *after* build, since the tests depend on `/tmp/QS/Configuration/Quicksilver.pch`
- Split uploads into separate named actions
- Copy the codesigned app to parent directory for easy acess
- Create a zip of QS.app as a convenience build artifact
- Specify release config for testing
- Use `working-directory` instead of `cd` for several actions
- Rename `release.yaml` to `ci.yaml` as it now has separate stages for
  build, sign, and release
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 a pull request may close this issue.

3 participants