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

Split signing out from qsrelease #2655

Merged
merged 1 commit into from
Mar 3, 2022
Merged

Split signing out from qsrelease #2655

merged 1 commit into from
Mar 3, 2022

Conversation

n8henrie
Copy link
Member

Issues addressed:

  • Provides separate GitHub Actions for building and signing (Tests fail with cycle in dependencies #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

TODO:

Fixes #2654 -- hopefully

@n8henrie
Copy link
Member Author

https://github.com/actions/upload-artifact#zipped-artifact-downloads

One of the consequences of this limitation is that if a zip is uploaded during a workflow run and then downloaded from the UI, there will be a double zip created.

A little annoying.

@n8henrie
Copy link
Member Author

Blocked on actions/upload-artifact#300

@n8henrie n8henrie force-pushed the split-signing branch 2 times, most recently from 86167af to bc87681 Compare February 24, 2022 20:08
@n8henrie
Copy link
Member Author

n8henrie commented Feb 24, 2022

OK, I think this is as ready as I can get it for now. I've squashed and rebased.

  • I'm blocked on debugging locally unless @skurfer can add me to the apple "team": Quicksilver is not codesigned #2654 (comment)
  • Whether or not I'm added to the team for local debugging, I don't have access to manage the repo's GitHub Secrets. In addition to the 3 added here, we will need the 4 listed here:
    • SIGNING_IDENTITY
    • NOTARIZING_ID
    • NOTARIZING_PASS
    • NOTARIZING_TEAM_ID

@n8henrie
Copy link
Member Author

n8henrie commented Feb 24, 2022

Actually, the TEAM_ID and SIGNING_IDENTITY can be the same thing (I think), as long as it's set to the 1234ABCD below, so just 3 secrets to add:

$ security find-identity -p basic -v
"Developer ID Application: Your Name (1234ABCD)"
  • SIGNING_IDENTITY
  • NOTARIZING_ID
  • NOTARIZING_PASS

@pjrobertson
Copy link
Member

pjrobertson commented Feb 25, 2022

Looks like build / sign is failing:

Error: Team ID must be at least 3 characters

@n8henrie
Copy link
Member Author

n8henrie commented Feb 25, 2022

Correct -- the 3 secrets listed above are currently empty, and I don't have access to modify repo secrets.

This is blocked pending @skurfer's review and feedback, since it's his Apple ID and cert.

@skurfer
Copy link
Member

skurfer commented Feb 26, 2022

I’ve made you an Owner on the Quicksilver org @n8henrie, so you should be able to make whatever changes are necessary now.

@skurfer
Copy link
Member

skurfer commented Feb 26, 2022

I’ve added the secrets and updated the signing cert to match the one I generated recently.

@n8henrie n8henrie force-pushed the split-signing branch 4 times, most recently from c2b0659 to 32a5a4e Compare February 27, 2022 02:21
@n8henrie
Copy link
Member Author

n8henrie commented Feb 28, 2022

I'm making some progress here.

Current errors with notarization:

{
  "severity": "error",
  "code": null,
  "path": "Quicksilver 1.6.1.dmg/Quicksilver.app/Contents/Resources/QSDroplet.app/Contents/MacOS/QSDroplet",
  "message": "The binary is not signed with a valid Developer ID certificate.",
  "docUrl": null,
  "architecture": "x86_64"
},
{
  "severity": "error",
  "code": null,
  "path": "Quicksilver 1.6.1.dmg/Quicksilver.app/Contents/Resources/QSDroplet.app/Contents/MacOS/QSDroplet",
  "message": "The signature does not include a secure timestamp.",
  "docUrl": null,
  "architecture": "x86_64"
},

I'm about to push a PR that might fix the timestamp issue, which I'm hoping might fix the issue with QSDroplet. Alternatively, it might need some different handling because it is an app itself.

What does QSDroplet do? Does it provide the ability to drag and drop something onto the QS icon?

Pending tasks:

  • Fix QSDroplet signing
  • Investigate opportunities to harden security and ensure no private info can be leaked in logs or by a maliciously crafted PR
    • I think limiting the sign action to only run on tags and not PRs will go a long way here, another advantage of splitting it into two actions
    • UPDATE: It appears that non-owner contributors have no access to secrets from their PRs, so perhaps this is less critical than I'd thought
  • Determine how we want the DMG to be named
    • Taking Quicksilver 1.6.1.dmg as an example, I think continuing to get the 1.6.1 from Info.plist is fine
    • 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

@skurfer
Copy link
Member

skurfer commented Feb 28, 2022

What does QSDroplet do? Does it provide the ability to drag and drop something onto the QS icon?

It has a different icon and would be a separate drop target from the main app (or potentially many targets). It’s sort of like a trigger, but you drop something on it.

https://qsapp.com/manual/Preferences/#command-objects-and-droplets

@n8henrie
Copy link
Member Author

Hmmm, same errors after adding --timestamp in 166b36f:

"issues": [
    {
      "severity": "error",
      "code": null,
      "path": "Quicksilver 1.6.1.dmg/Quicksilver.app/Contents/Resources/QSDroplet.app/Contents/MacOS/QSDroplet",
      "message": "The binary is not signed with a valid Developer ID certificate.",
      "docUrl": null,
      "architecture": "x86_64"
    },
    {
      "severity": "error",
      "code": null,
      "path": "Quicksilver 1.6.1.dmg/Quicksilver.app/Contents/Resources/QSDroplet.app/Contents/MacOS/QSDroplet",
      "message": "The signature does not include a secure timestamp.",
      "docUrl": null,
      "architecture": "x86_64"
    },
    {
      "severity": "error",
      "code": null,
      "path": "Quicksilver 1.6.1.dmg/Quicksilver.app/Contents/Resources/QSDroplet.app/Contents/MacOS/QSDroplet",
      "message": "The binary is not signed with a valid Developer ID certificate.",
      "docUrl": null,
      "architecture": "arm64"
    },
    {
      "severity": "error",
      "code": null,
      "path": "Quicksilver 1.6.1.dmg/Quicksilver.app/Contents/Resources/QSDroplet.app/Contents/MacOS/QSDroplet",
      "message": "The signature does not include a secure timestamp.",
      "docUrl": null,
      "architecture": "arm64"
    }
  ]

@n8henrie
Copy link
Member Author

$ codesign --verify -vvv Quicksilver.app/Contents/Resources/QSDroplet.app/
Quicksilver.app/Contents/Resources/QSDroplet.app/: valid on disk
Quicksilver.app/Contents/Resources/QSDroplet.app/: satisfies its Designated Requirement

codesign --verify --deep --strict --verbose=1 "${QS_VERSIONED_DMG}"

## Easy access to plist and app
cp -a "${DMG_TEMP}"/Quicksilver.app{,/Contents/Info.plist} "${BUILT_PRODUCTS_DIR}"
Copy link
Member

Choose a reason for hiding this comment

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

Why are we copying the app back to $BUILT_PRODUCTS_DIR here? That’s where it originates from, and it still exists, so the script fails for me as a result. We should just need Info.plist from this step.

Copy link
Member Author

@n8henrie n8henrie Feb 28, 2022

Choose a reason for hiding this comment

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

Doing a codesigning dance. The unsigned app is copied into BUILT_PRODUCTS_DIR; if QS_BUILD_ONLY is set, this copy is ad-hoc signed so that it can be run locally, and I left a copy in BUILT_PRODUCTS_DIR as a convenience to preserve existing behavior.

It is copied into DMG_TEMP so that this directory can be uploaded as a built artifact that can them be downloaded and manually explored if desired, but more importantly because artifacts within a single workflow can then be downloaded in other actions, avoiding the need to re-build everything.

In qssign, the app is codesigned, and the the "finished" app is subsequently re-copied to BUILT_PRODUCTS_DIR, overwriting the unsigned copy that was there previously

EDIT: so that you have convenient access to a signed copy when running locally. However, I could mv (instead of cp) it into DMG_TEMP and remove the Quicksilver.zip.zip build artifact, and people could just download the DMG_INGREDIENTS artifact (with a copy of the unsigned Quicksilver.app inside) -- it just seems a little less obvious for people wanting to get a copy of Quicksilver.app build from a PR. I could have the build artifact made from the one in DMG_TEMP -- several options. I didn't consider a duplicate copy to be a huge issue, but I can see how it could be confusing down the road.

I'm surprised that the script fails for you (thanks for testing) -- shouldn't cp -a just overwrite it?

Ahh --

$ cd $(mktemp -d)
$ mkdir ./dmg
$ /bin/cp -a /tmp/QS/build/Release/Quicksilver.app .
$ /bin/cp -a ./Quicksilver.app ./dmg/
$ /bin/cp -a ./dmg/Quicksilver.app .
cp: cannot overwrite directory ./
... lots of errors ...
$ which cp
/opt/homebrew/opt/coreutils/libexec/gnubin/cp
$ cp -a ./dmg/Quicksilver.app .
$ echo $?
0

The issue didn't come up on GitHub Actions because the environments are cleared between the two actions (BUILT_PRODUCTS_DIR is unpopulated in qssign).

I reran with PATH=$(getconf PATH) to make sure no more GNU / BSD coreutils issues.

@n8henrie n8henrie force-pushed the split-signing branch 2 times, most recently from d238534 to 0d5249b Compare March 1, 2022 00:30
@n8henrie n8henrie force-pushed the split-signing branch 3 times, most recently from 156a697 to 1023466 Compare March 1, 2022 21:43
@n8henrie
Copy link
Member Author

n8henrie commented Mar 2, 2022

A few other conveniences provided by releases:

The canonical URL to the asset is https://github.com/quicksilver/Quicksilver/releases/download/<tag-name>/<asset-name>, but you'll get evergreen redirects with:

For better or worse the version number in the DMG prevents one from getting an "always working direct download url."

@n8henrie n8henrie force-pushed the split-signing branch 2 times, most recently from 1eb16ce to d590b8a Compare March 3, 2022 00:36
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
@skurfer skurfer merged commit 90328fc into master Mar 3, 2022
@skurfer skurfer deleted the split-signing branch March 3, 2022 15:35
skurfer added a commit that referenced this pull request Mar 3, 2022
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.

Quicksilver is not codesigned
3 participants