-
Notifications
You must be signed in to change notification settings - Fork 32
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
feat: macos signing and notarization #367
Conversation
d891669
to
a06abf7
Compare
057619f
to
2ba7177
Compare
7183e8a
to
92386a4
Compare
2852464
to
9544654
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ready for review (silence is compliance – aiming for merging in the beginning of the next week).
'{ state: $state, target_url: $target_url, description: $description, context: $context }' ) | ||
curl --output /dev/null --silent --show-error \ | ||
-X POST -H "Authorization: Bearer $GITHUB_TOKEN" -H 'Content-Type: application/json' \ | ||
--data "$API_PARAMS" 'https://api.github.com/repos/ipfs/distributions/statuses/${GIT_REVISION}' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ℹ️ we could replace this script with JS call and actions/github-script
but I don't want to sink any more time into refactoring this PR ;)
docker build . -t distributions \ | ||
--build-arg CACHEBUST=`date --iso-8601=date` \ | ||
--build-arg USER_UID=$(id -u "$USER") \ | ||
--build-arg GO_IPFS_VER=${GO_IPFS_VER:-$(curl -s https://dist.ipfs.io/go-ipfs/versions | tail -n 1)} # match http api client version on CI |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ℹ️ this controls the version of the CLI client inside ./dockerized
build, which talks to go-ipfs running outside on the host. Here, we ensure it uses the same version (with fallback to the latest one if GO_IPFS_VER
is undefined).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this comment live in the source file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe the comment on the very right is enough, more context can be found on this PR.
if [[ $(jq '.peer_map[].status' cluster-pin-status | grep '"pinned"' | wc -l) -ge 2 ]]; then | ||
echo "Got 2 pin confirmations, finishing the workflow" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ℹ️ to make CI faster and avoid hanging forever if parts of cluster are out of sync, cluster pinning is considered "done" after we have 2 copies.
Here, I do it manually in userland, but filled ipfs-cluster/ipfs-cluster#1427 to add this sort of thing to ipfs-cluster-ctl CLI tool.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to above: when do we decide to put this in the PR vs. the code itself? Do folks tend to do a git blame and look for comments to find more context?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I often use git blame, but in this specific case those comments are provided mostly to make PR review easier.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. I'll defer to the team for the standard. I was bringing this up because it was hard for me to imagine a case where a comment is useful during PR where it isn't also useful 6 months in the future when looking at the code with fresh eyes. Anyways, sounds good.
# fix resolv - DNS provided by Github is unreliable for DNSLik/dnsaddr | ||
sudo sed -i -e 's/nameserver 127.0.0.*/nameserver 1.1.1.1/g' /etc/resolv.conf |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ℹ️ This was pretty annoying. Entire build failed because Githubs DNS proxying cache at 127.0.0.*
randomly timeouted while resolving _dnslink
or _dnsaddr
. Switching to Cloudflare here make the problem go away.
EXECUTABLES=$(jq -nc '$ARGS.positional' --args $(find "./tmp/${DIST_NAME}_${DIST_VERSION}_${arch}-unsigned/" -perm +111 -type f -print)) | ||
echo "{ | ||
\"source\" : $EXECUTABLES, | ||
\"bundle_id\" : \"io.ipfs.dist.${DIST_NAME}\", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ℹ️ I registered the wildcard bundle_id=io.ipfs.dist.*
at Apple, which enables us to have each package "isolated" from each other. This is important in case Apple decides to block something from running on macOS – we don't have all egs in one basket.
brew install ipfs coreutils gawk gnu-sed jq mitchellh/gon/gon | ||
ipfs init --profile test # needed for calculating NEW_CID later | ||
- name: Import Keychain Certs | ||
uses: apple-actions/import-codesign-certs@253ddeeac23f2bdad1646faac5c8c2832e800071 # v1@2020-02-03 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ℹ️ 👮♂️ I hardcoded specific revision because this is a third-party action and in theory someone could swap-out git tag and steal our signing keys.
brew tap mitchellh/gon | ||
brew install ipfs coreutils gawk gnu-sed jq mitchellh/gon/gon |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ℹ️ gon package is used by Terraform project, so multiple eyes are looking at this
To produce a signed, **official build** for use in DNSLink at `dist.ipfs.io`: | ||
|
||
1. Run `./dist.sh add-version` locally. | ||
2. Commit created changes to `dists/<dist>` and open a PR against `ipfs/distributions`. | ||
3. Wait for Github Action to finish PR build. It runs `./dockerized` build, then signs macOS binaries and spits out updated root CID at the end. | ||
4. If everything looks good, write down the CID from the preview link on the PR, and update the DNSlink at `dist.ipfs.io`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ℹ️ I believe this is the TLDR of this entire PR :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, although I mostly reviewed the README. We may learn more as we start using it for nightlies though ❤️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work!
Only comments were around inlining comments PR into the code for discoverability. Your call.
docker build . -t distributions \ | ||
--build-arg CACHEBUST=`date --iso-8601=date` \ | ||
--build-arg USER_UID=$(id -u "$USER") \ | ||
--build-arg GO_IPFS_VER=${GO_IPFS_VER:-$(curl -s https://dist.ipfs.io/go-ipfs/versions | tail -n 1)} # match http api client version on CI |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this comment live in the source file?
if [[ $(jq '.peer_map[].status' cluster-pin-status | grep '"pinned"' | wc -l) -ge 2 ]]; then | ||
echo "Got 2 pin confirmations, finishing the workflow" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to above: when do we decide to put this in the PR vs. the code itself? Do folks tend to do a git blame and look for comments to find more context?
+ add ability to run workflow against arbitrary path - until we have signed nightlies
As agreed during stewards call, I'm starting the merge dance starting with this PR |
This PR adds
sign-macos
job to CI workflow, adding a transparent signing step.The main reason is to sign repo migrations and avoid issue from ipfs/kubo#8240 globally (outside of Brave). It also provides us with a template if we wish to add signing for other platforms.
How this works?
Technical details
This PR splits
make publish
performed inmain.yml
into three stages:build
executes./dockerized make all_dists
and produces any missing binaries in./releases
sign-macos
signs and notarizes every new binary from./releases
and updates.tar.gz
packages and relevant hashes in-placepublish
make publish
and adds any new and signed packages from./releases
toDIST_ROOT
(
/ipns/dist.ipfs.io
by default)Flow for adding new dist or version
./dists/{name}/versions
DIST_ROOT
to ipfs-clusterpublish
step of CI buildContent routing optimization
IPFS node running on CI manually pre-connects to ipfs-websites cluster peers, which are known to already have all blocks required during dist.ipfs.io build. This way we skip DHT lookup.
Website preview on PRs
Every PR now gets a link to website preview at the
dweb.link
IPFS gateway:TODO
/dnsaddr/ipfs-websites.collab.ipfscluster.io
but it errors right nowDIST_ROOT
and ensure CI is no-op when no new binaries are added