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

[bug] Opaque failure in final step #3031

Closed
woodruffw opened this issue Dec 12, 2023 · 8 comments · Fixed by #3313 or #3456
Closed

[bug] Opaque failure in final step #3031

woodruffw opened this issue Dec 12, 2023 · 8 comments · Fixed by #3313 or #3456
Labels
area:generic Issue with the generic generator type:bug Something isn't working type:documentation Improvements or additions to documentation
Milestone

Comments

@woodruffw
Copy link

Describe the bug

I'm attempting to publish a new version of the Python id package, using the package's GitHub Actions release workflow.

This is what our current provenance generation step looks like:

  generate-provenance:
    needs: [build]
    name: Generate build provenance
    permissions:
      actions: read   # To read the workflow path.
      id-token: write # To sign the provenance.
      contents: write # To add assets to a release.
    # Currently this action needs to be referred by tag. More details at:
    # https://github.com/slsa-framework/slsa-github-generator#verification-of-provenance
    uses: slsa-framework/slsa-github-generator/.github/workflows/generator_generic_slsa3.yml@v1.9.0
    with:
      attestation-name: provenance-id-${{ github.event.release.tag_name }}.intoto.jsonl
      base64-subjects: "${{ needs.build.outputs.hashes }}"
      compile-generator: true # Workaround for https://github.com/slsa-framework/slsa-github-generator/issues/1163
      upload-assets: true

When our release workflow is triggered, this step runs as expected, and the interior detect-env and generator steps appear to succeed. The upload-assets step is then skipped, and final fails opaquely (apparently checking some kind of success state, without an error message):

  echo "outcome=$([ "$SUCCESS" == "true" ] && echo "success" || echo "failure")" >> "$GITHUB_OUTPUT"
  [ "$CONTINUE" == "true" ] || [ "$SUCCESS" == "true" ] || exit 27

You can see that in the release workflow logs here: https://github.com/di/id/actions/runs/7184211112/job/19564897464

This error persists even when the job is re-run.

To Reproduce

I don't have an easy reproducer, unfortunately. I'm happy to perform debugging steps on the di/id repository, however.

Expected behavior

I expected the generate-provenance step to succeed, as it has for previous release runs.

@woodruffw woodruffw added status:triage Issue that has not been triaged type:bug Something isn't working labels Dec 12, 2023
@woodruffw
Copy link
Author

One possible hint: it looks like the reusable workflow uses actions/upload-artifact internally, which is failing with:

Run actions/upload-artifact@0b7f8abb1508181956e8e162db84b466c27e18ce
Error: Input required and not supplied: path

as seen here: https://github.com/di/id/actions/runs/7184211112/job/19564896850#step:7:10

(I didn't catch this originally because the generator step that it runs in still shows as "green", despite that failure.)

@woodruffw
Copy link
Author

Some more digging: we currently use attestation-name, which appears to be marked as deprecated in 1.9.0 in favor of provenance-name. However it looks like the failing upload-artifact step only knows about provenance-name. So that might be the source of the issue.

@laurentsimon
Copy link
Collaborator

I think you're right, that seems to be the problem https://github.com/slsa-framework/slsa-github-generator/blob/main/.github/workflows/generator_generic_slsa3.yml#L256. Have you confirmed that provenance-name fixes the problem?

We need to update the documentation

@woodruffw
Copy link
Author

Have you confirmed that provenance-name fixes the problem?

Not yet, I'll try that in a moment. But yeah, I think this needs a doc update, and ideally a few other things if possible:

  • It looks like attestation-name was deprecated but the behavior change wasn't intended (?) until deprecation is finished, so a fix might be necessary. Otherwise, it's less a deprecation and more of a behavior change within a minor version 😅
  • For the deprecation, it might be nice to emit a warning or similar GitHub annotation to tell users that they need to update their workflows

@woodruffw
Copy link
Author

Opened di/id#147 with the prospective fix -- @laurentsimon would you be able to give that a look?

@woodruffw
Copy link
Author

Looks like the fix works as expected! I'll leave this open for the documentation side 🙂

@ianlewis ianlewis added area:generic Issue with the generic generator type:documentation Improvements or additions to documentation and removed status:triage Issue that has not been triaged labels Jan 9, 2024
@ianlewis
Copy link
Member

I added #3071 to improve error messages and #3072 to add a deprecation warning to attestation-name.

For this bug I think we should fix the attestation-name backwards compatibility.

@laurentsimon laurentsimon added this to the Next release milestone Jan 16, 2024
@mihaimaruseac
Copy link
Contributor

mihaimaruseac commented Feb 6, 2024

I think this is why we're seeing failure in model_transparency SLSA for ML too. Will try to send a fix

This was wrong, the issue was the way we were computing the digest on MacOS/Windows.

laurentsimon pushed a commit that referenced this issue Mar 13, 2024
# Summary

- Fixes #3031
- Fixes #3072 
- Removes the `attestation-name` input and output from the
`generator_generic_slsa3.yml`, which has been deprecated for
`provenance-name`.

## Testing Process

We cannot properly test workflow changes without first merging to
`main`, and revert if tests fail. After merging we will

- [ ] trigger a manual run of `pre-submit e2e generic default`
- [ ] wait for nightly e2e tests to report success

## Checklist

- [x] Review the contributing [guidelines](./../CONTRIBUTING.md)
- [x] Add a reference to related issues in the PR description.
- [x] Update documentation if applicable.
- [ ] Add unit tests if applicable.
- [x] Add changes to the [CHANGELOG](./../CHANGELOG.md) if applicable.

---------

Signed-off-by: Ramon Petgrave <ramon.petgrave64@gmail.com>
laurentsimon pushed a commit that referenced this issue Mar 27, 2024
# Summary

- Reverting #3399
- Fixes
#3031
- Fixes
#3072
- Removes the attestation-name input and output from the
generator_generic_slsa3.yml, which has been deprecated for
provenance-name.

## Testing Process

- We have existing PR Check workflows that do call the generic-genertor
wth the correct parameters
- example-package e2e2 tests have already been updated to use the new
parameter and are already passing.

## Checklist

- [x] Review the contributing [guidelines](./../CONTRIBUTING.md)
- [x] Add a reference to related issues in the PR description.
- [x] Update documentation if applicable.
- [x] Add unit tests if applicable.
- [x] Add changes to the [CHANGELOG](./../CHANGELOG.md) if applicable.

Signed-off-by: Ramon Petgrave <ramon.petgrave64@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:generic Issue with the generic generator type:bug Something isn't working type:documentation Improvements or additions to documentation
Projects
None yet
4 participants