-
Notifications
You must be signed in to change notification settings - Fork 362
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: verify provenance #309
feat: verify provenance #309
Conversation
bf21438
to
f083c64
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.
thanks for this contribution! just some questions for both you and @laurentsimon and @ianlewis :)
uses: ./.github/actions/test.yaml | ||
- name: Run Lints | ||
uses: ./.github/actions/lint.yaml | ||
# - name: Run Tests |
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.
Can we bring these back? This is an important part of the release process.
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.
the reason why I did this is that workflow didn't worked as it didn't find these action files under .github/actions
.
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.
Ah my bad, fixing this in #311.
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.
@another-rex I think the problem still continues. I also do not think that this is the way of referencing different workflows within 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.
You're right, I misunderstood how the uses line works. I updated and tested it in #315, which seems to work in my fork.
set -euo pipefail | ||
TAG=${GITHUB_REF#refs/tags/} |
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.
There's a fair bit of bash here to maintain :)
@laurentsimon @ianlewis is this recommended practice? If so, would it be possible at all to generalize this in a more re-usable way "upstream" somewhere as part of a slsa-framework repo?
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.
Yeah, it might be a good idea to have an action for verifying artifacts on a GitHub release. i.e. Given a release download all the artifact and verify them.
This would only work for a situation like osv-scanner where you provide binary artifacts along with the provenance (as opposed to some other language package) but I think it could still be useful.
We would also need to be careful to provide a way to provide the list of expected artifacts. We would need to catch the situation where an artifact was excluded from the provenance even though it was expected to be there (in this case goreleaser.outputs.hashes
)
/cc @laurentsimon
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 can work on this 🤩
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 is a wonderful idea we should discuss this topic
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.
@ianlewis @laurentsimon thoughts on this going into some slsa-framework repo instead (either a new one or existing?)
I would very much prefer that approach as it makes this work a lot more useful and we (OSV-Scanner maintainers) have less things we're not as familiar with to maintain :)
echo "Downloading provenance $PROVENANCE" | ||
gh -R $GITHUB_REPOSITORY release download --clobber $GITHUB_REF_NAME -p $PROVENANCE |
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.
If I'm understanding this code properly, the provenance would be already downloaded as it's also a release asset.
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 think the code from https://github.com/google/go-containerregistry/blob/main/.github/workflows/release.yml#L48-L76 should work just fine, no?
We can make the attestation name dynamic instead of hard-coding it
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.
here is the PR for that: google/go-containerregistry#1611
gh release download
didn't work with draft releases as osv-scanner marks its release as draft and I looked for a solution for gh release download
to enable downloading assets from draft release but I couldn't find anything useful, then I turned the solution back to the GitHub API calls via gh api
. But let me try again to ensure that I didn't work.
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.
it worked btw with the right permissions, so to download releases it requires you to put write permissions for content. Great!
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.
that's interesting. Maybe file an issue on GitHub.. sounds like a bug... they should have a REST flag to allow for draft release to be fetched, without the need for a write token.
set -euo pipefail | ||
TAG=${GITHUB_REF#refs/tags/} |
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.
Yeah, it might be a good idea to have an action for verifying artifacts on a GitHub release. i.e. Given a release download all the artifact and verify them.
This would only work for a situation like osv-scanner where you provide binary artifacts along with the provenance (as opposed to some other language package) but I think it could still be useful.
We would also need to be careful to provide a way to provide the list of expected artifacts. We would need to catch the situation where an artifact was excluded from the provenance even though it was expected to be there (in this case goreleaser.outputs.hashes
)
/cc @laurentsimon
.github/workflows/goreleaser.yml
Outdated
gh api "repos/developer-guy/osv-scanner/releases" | ||
echo $GITHUB_REPOSITORY |
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 assume this is debugging code and you'll want to remove it?
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.
yep, I put this for debugging purposes, it should be removed.
.goreleaser.yml
Outdated
@@ -32,7 +32,7 @@ builds: | |||
dockers: | |||
# Arch: amd64 | |||
- image_templates: | |||
- 'ghcr.io/google/osv-scanner:{{ .Tag }}-amd64' | |||
- 'ghcr.io/{{ .Env.ACTOR }}/osv-scanner:{{ .Tag }}-amd64' |
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.
Is this right? I'm not sure that the github.actor
is going to be right if the repo is owned by an organization.
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 might use github.repository_owner
instead, will it work?
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 renamed it as OWNER
and used github.repository_owner
.
for i in $(gh api "repos/$GITHUB_REPOSITORY/releases" | jq -r '.[] | select(.tag_name=='\"$TAG\"').assets[].id') | ||
do | ||
echo "Downloading asset $i" | ||
curl -L -o $(gh api "repos/$GITHUB_REPOSITORY/releases/assets/$i" | jq -r '.name') -H "Authorization: token $GH_TOKEN" -H "Accept: application/octet-stream" $(gh api "repos/$GITHUB_REPOSITORY/releases/assets/$i" | jq -r '.url') | ||
done |
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.
Is there a reason you can't just do gh release download "$TAG"
?
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.
for the same reason I explained above: #309 (comment)
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.
it worked @ianlewis with right permisssions.
49c0718
to
0105759
Compare
a453bfc
to
ea97873
Compare
needs: [goreleaser, provenance] | ||
runs-on: ubuntu-latest | ||
permissions: | ||
contents: write # To add assets to a release. |
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.
please file a bug on https://github.com/actions/runner. Requiring write permission is really odd
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.
let me try it again, I changed back it to "read".
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.
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.
it didn't work with "write" permission either:
https://github.com/developer-guy/osv-scanner/actions/runs/4519341906/jobs/7959797410#step:1:17
but it worked once what is going on 🤔
https://github.com/developer-guy/osv-scanner/actions/runs/4504207210/jobs/7928457868#step:1:17
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 script is just working fine with write
permission: https://github.com/developer-guy/osv-scanner/actions/runs/4519410595/jobs/7959907135
set -euo pipefail
TAG=${GITHUB_REF#refs/tags/}
echo "Downloading assets for tag $TAG"
for i in $(gh api "repos/$GITHUB_REPOSITORY/releases" | jq -r '.[] | select(.tag_name=='\"$TAG\"').assets[].id')
do
echo "Downloading asset $i"
curl -L -o $(gh api "repos/$GITHUB_REPOSITORY/releases/assets/$i" | jq -r '.name') -H "Authorization: token $GH_TOKEN" -H "Accept: application/octet-stream" $(gh api "repos/$GITHUB_REPOSITORY/releases/assets/$i" | jq -r '.url')
done
echo "Downloading provenance $PROVENANCE"
gh -R $GITHUB_REPOSITORY release download --clobber $GITHUB_REF_NAME -p $PROVENANCE
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
ea97873
to
a84cfd9
Compare
another thing that we need to solve is supporting draft releases in SLSA 3 Generic Generator because, at the moment of time, we are going to have two same releases but one is draft and another is the latest. PTAL @laurentsimon @ianlewis |
ada8483
to
ff4ef7f
Compare
Signed-off-by: Batuhan Apaydın <batuhan.apaydin@trendyol.com> Co-authored-by: Furkan Türkal <furkan.turkal@trendyol.com> Signed-off-by: Batuhan Apaydın <batuhan.apaydin@trendyol.com>
ff4ef7f
to
c4c262a
Compare
This pull request has not had any activity for 60 days and will be automatically closed in two weeks |
Automatically closing stale pull request |
Signed-off-by: Batuhan Apaydın batuhan.apaydin@trendyol.com
Co-authored-by: Furkan Türkal furkan.turkal@trendyol.com
PTAL @laurentsimon @ianlewis