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

feat: verify provenance #309

Conversation

developer-guy
Copy link

Signed-off-by: Batuhan Apaydın batuhan.apaydin@trendyol.com
Co-authored-by: Furkan Türkal furkan.turkal@trendyol.com

PTAL @laurentsimon @ianlewis

@developer-guy developer-guy force-pushed the feature/provenance-verification branch from bf21438 to f083c64 Compare March 22, 2023 20:51
Copy link
Collaborator

@oliverchang oliverchang left a 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
Copy link
Collaborator

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.

Copy link
Author

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.

Copy link
Collaborator

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.

Copy link
Author

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.

Screen Shot 2023-03-25 at 3 29 07 PM

Copy link
Collaborator

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/}
Copy link
Collaborator

@oliverchang oliverchang Mar 23, 2023

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?

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

Copy link
Author

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 🤩

Copy link
Author

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

Copy link
Collaborator

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

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.

Copy link

@laurentsimon laurentsimon Mar 23, 2023

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

Copy link
Author

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.

Copy link
Author

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!

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/}

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

Comment on lines 60 to 61
gh api "repos/developer-guy/osv-scanner/releases"
echo $GITHUB_REPOSITORY

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?

Copy link
Author

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'

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.

Copy link
Author

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?

Copy link
Author

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.

Comment on lines +90 to +93
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

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" ?

Copy link
Author

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)

Copy link
Author

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.

@developer-guy developer-guy force-pushed the feature/provenance-verification branch 3 times, most recently from 49c0718 to 0105759 Compare March 24, 2023 12:01
@developer-guy developer-guy changed the title verify provenance feat: verify provenance Mar 24, 2023
@developer-guy developer-guy force-pushed the feature/provenance-verification branch 2 times, most recently from a453bfc to ea97873 Compare March 24, 2023 12:18
needs: [goreleaser, provenance]
runs-on: ubuntu-latest
permissions:
contents: write # To add assets to a release.
Copy link

@laurentsimon laurentsimon Mar 24, 2023

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

Copy link
Author

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".

Copy link
Author

@developer-guy developer-guy Mar 25, 2023

Choose a reason for hiding this comment

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

Copy link
Author

Choose a reason for hiding this comment

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

Copy link
Author

@developer-guy developer-guy Mar 25, 2023

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

Copy link

@laurentsimon laurentsimon left a comment

Choose a reason for hiding this comment

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

LGTM

@developer-guy developer-guy force-pushed the feature/provenance-verification branch from ea97873 to a84cfd9 Compare March 25, 2023 12:25
@developer-guy
Copy link
Author

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

slsa-framework/slsa-github-generator#1476

@developer-guy developer-guy force-pushed the feature/provenance-verification branch 10 times, most recently from ada8483 to ff4ef7f Compare March 27, 2023 20:10
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>
@developer-guy developer-guy force-pushed the feature/provenance-verification branch from ff4ef7f to c4c262a Compare March 27, 2023 20:15
Copy link

This pull request has not had any activity for 60 days and will be automatically closed in two weeks

@github-actions github-actions bot added the stale The issue or PR is stale and pending automated closure label Jul 23, 2024
Copy link

github-actions bot commented Aug 6, 2024

Automatically closing stale pull request

@github-actions github-actions bot added the autoclosed Closed by automation label Aug 6, 2024
@github-actions github-actions bot closed this Aug 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autoclosed Closed by automation stale The issue or PR is stale and pending automated closure
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants