slsa-framework / slsa-github-generator

Language-agnostic SLSA provenance generation for Github Actions
Apache License 2.0
413 stars 127 forks source link

[bug] Problem using SLSA3+ provenance generator for arbitrary projects #731

Open rbehjati opened 2 years ago

rbehjati commented 2 years ago

Describe the bug I am following the instructions for using generator_generic_slsa3 to generate provenances for Oak. The call to the workflow fails with the following error:

Run ./.github/actions/generate-builder/generate-builder.sh
/home/runner/work/_temp/9c89bedb-16a3-4329-be6c-a854f00c4572.sh: line 1: ./.github/actions/generate-builder/generate-builder.sh: No such file or directory
Error: Process completed with exit code 127.

The workflow is running and failing on a pull_request trigger. I know that pull_request triggers are not supported, but since this is the first time I am using this workflow, I was hoping to experiment with it before merging this PR to main. If this is the expected behaviour on pull requests, it would help to have the behaviour described in the documentation.

To Reproduce Here is the PR that uses the generator_generic_slsa3 workflow: https://github.com/project-oak/oak/pull/3166 This is the failed action: https://github.com/project-oak/oak/runs/7953028722?check_suite_focus=true. The step that fails is the "Generate builder" step.

Screenshots

Screen Shot 2022-08-22 at 14 55 49

asraa commented 2 years ago

It is true that this happens because of the pull_request trigger. Because of this, the workflow can't access the OIDC token required to create the Fulcio issued certificate.

Can we support a dry-run option that is only valid for pull_request triggers?

It depends what you would like to test here: If you want to test provenance generation, but not signature validation, we can do something similar to how the CI tests here work in which they do not actually create sigs.

rbehjati commented 2 years ago

A dry-run option would be great. For the pull-request, I only care about provenance generation.

asraa commented 2 years ago

That would be an easy fix, we can modify IsPresubmitTests(): https://github.com/slsa-framework/slsa-github-generator/blob/1a55da84338004ca26d38b941483c89ab93a695d/internal/utils/presubmits.go#L20

@laurentsimon any oppposition to allowing all pull_request triggers here?

laurentsimon commented 2 years ago

pull_request is already listed, is it not?

asraa commented 2 years ago

pull_request is already listed, is it not?

Must be pull_request AND this repository, I can change to remove the AND

laurentsimon commented 2 years ago

Got it. Is this something we should give an option for so that's it's more explicit? I'm worried users won't realize that signature fails.

/cc @ianlewis

asraa commented 2 years ago

Yes, exactly: only if a dry-run input option is passed in.

laurentsimon commented 2 years ago

That'd work for me. So the dry-run would generate the signatures on triggers that support it, and not for triggers like pull_request, or no generation in both cases?

asraa commented 2 years ago

So the dry-run would generate the signatures on triggers that support it, and not for triggers like pull_request, or no generation in both cases?

I was thinking yes, that it would end up making the workflow compatible with pull_request when run in that.

I agree there's a risk that users may assume this would be valid provenance. No upload or no new release would be done in the case that a dry run is being performed.

ianlewis commented 2 years ago

I think having a dry-run option is fine if we document it. We should also probably put in some kind of warning in the output that the provenance is not signed.

I'm thinking:

rbehjati commented 2 years ago

Got it. Is this something we should give an option for so that's it's more explicit? I'm worried users won't realize that signature fails.

Does the signature actually fail, or is it possible to skip it? My assumption was that the provenance is first generated, and then signed in a separate, possibly optional step. If the two must happen together, perhaps the dry-run does not even need to generate the provenance, but just give a signal the the workflow job is set up correctly. For instance, for my PR, what I want to know is that I have configured the permissions correctly, and am passing in the right inputs.

For the permissions, by the way, I had to change contents: read to contents: write.

Also, the current error message says ./.github/actions/generate-builder/generate-builder.sh: No such file or directory. That does not look like a permission or access problem to me.

laurentsimon commented 2 years ago

Got it. Is this something we should give an option for so that's it's more explicit? I'm worried users won't realize that signature fails.

Does the signature actually fail, or is it possible to skip it? My assumption was that the provenance is first generated, and then signed in a separate, possibly optional step. If the two must happen together, perhaps the dry-run does not even need to generate the provenance, but just give a signal the the workflow job is set up correctly.

Would using the workflow_dispatch trigger solve the problem? You need to merge it in first, but you can then trigger it manually. That will catch permission issues or other problems. You can then download the provenance from the artifacts (when viewing the Action run).

For the permissions, by the way, I had to change contents: read to contents: write.

We fixed the documentation yesterday.

rbehjati commented 2 years ago

Would using the workflow_dispatch trigger solve the problem?

Good idea. I'll try that, and will update this issue with the result. I think a dry-run option is still a better solution :)

We fixed the documentation yesterday.

Nice. Thanks.

rbehjati commented 2 years ago

Thanks for the help. I was able to use the workflow and generate a provenance. My first attempt failed, because I had not set the subject correctly. That is the kind of issue that a dry-run option can detect :)

Should I close this bug, or do you prefer to keep it open for more discussion about a dry-run option? Perhaps https://github.com/slsa-framework/slsa-github-generator/issues/358 is a better place for that discussion?

laurentsimon commented 2 years ago

Let's keep here. Dry-run option is more general than the pull_request. Thanks again for the issue

ianlewis commented 2 years ago

Related #131 #358 #124