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

Attest package provenance #12333

Merged
merged 5 commits into from
May 17, 2024
Merged

Attest package provenance #12333

merged 5 commits into from
May 17, 2024

Conversation

nicoddemus
Copy link
Member

This uses the new build provenance support added in build-and-inspect-python-package 2.5.0.

More information: https://github.blog/2024-05-02-introducing-artifact-attestations-now-in-public-beta/

Tested also in pytest-dev/pytest-mock#431.

Note: even though it is technically necessary only for the deploy workflow, as the test workflow does not publish its packages, decided to always attest the provenance in both cases to avoid any surprises related to this (say a misconfiguration) when deploying.

nicoddemus and others added 2 commits May 16, 2024 08:08
This uses the new build provenance support added in [build-and-inspect-python-package 2.5.0](https://github.com/hynek/build-and-inspect-python-package/blob/main/CHANGELOG.md#250---2024-05-13).

More information: https://github.blog/2024-05-02-introducing-artifact-attestations-now-in-public-beta/

Tested also in pytest-dev/pytest-mock#431.

Note: even though it is technically necessary only for the `deploy` workflow, as the `test` workflow does not publish its packages, decided to always attest the provenance in both cases to avoid any surprises related to this (say a misconfiguration) when deploying.
@nicoddemus
Copy link
Member Author

@bluetech
Copy link
Member

LGTM. I tested that I can verify the files, and also that if I corrupt one of the files it doesn't verify.

Maybe add a release note telling people that we're now doing that? (Perhaps call it "experimental")

Note: even though it is technically necessary only for the deploy workflow, as the test workflow does not publish its packages, decided to always attest the provenance in both cases to avoid any surprises related to this (say a misconfiguration) when deploying.

I think maybe we shouldn't create "useless" attestations like these:

  • It feels to me like something that should only be done for files that are truly intended to be distributed, this way we have the property "distributed <=> signed" rather than just "distributed => signed" which seems vaguely useful.
  • Each attestation like this will basically be stored in several immutable stores forever and it seems maybe not so nice to do it for every random workflow run...
  • The commit in the attestation seems to lead nowhere, I guess it's because it's a PR?

@nicoddemus
Copy link
Member Author

nicoddemus commented May 16, 2024

The commit in the attestation seems to lead nowhere, I guess it's because it's a PR?

Hmm that's strange, perhaps that's the merge commit? 🤔

EDIT:

image

Yep, that's 3fd5b86 from this branch merged with fdf3aa3 (main when that executed).

For our actual releases this should not be a problem, as they are triggered directly from the branch, not from a PR.

@nicoddemus
Copy link
Member Author

Maybe add a release note telling people that we're now doing that? (Perhaps call it "experimental")

Done, wording suggestions are welcome.

It feels to me like something that should only be done for files that are truly intended to be distributed, this way we have the property "distributed <=> signed" rather than just "distributed => signed" which seems vaguely useful.

Each attestation like this will basically be stored in several immutable stores forever and it seems maybe not so nice to do it for every random workflow run...

Good points, undid it for test.yml.

changelog/12333.trivial.rst Outdated Show resolved Hide resolved
Co-authored-by: Ran Benita <ran@unusedvar.com>
@nicoddemus nicoddemus merged commit 635fbe2 into main May 17, 2024
23 checks passed
@nicoddemus nicoddemus deleted the attest-build-provenance branch May 17, 2024 11:19
@webknjaz
Copy link
Member

@nicoddemus @bluetech @RonnyPfannschmidt FYI I consider this kind of privilege management dangerous as it silently creates opportunities for vulnerabilities in places you wouldn't think of (the attack surface would be other OIDC-connected services).
So here's my concerns on why this coupled implementation is dangerous: hynek/build-and-inspect-python-package#105 (comment).

@nicoddemus
Copy link
Member Author

@webknjaz thanks for raising this.

I read the comment and I think I kind understand the issue, but what would be the fix moving forward exactly? IIUC we build the package using the action, to fix this we would need to build the package directly without using the action at all?

@RonnyPfannschmidt
Copy link
Member

@nicoddemus attestation should happen in the upload job thats not touching external deps

@nicoddemus
Copy link
Member Author

nicoddemus commented Jun 24, 2024

Ahh of course, it does not affect the packages themselves, the provenance attestation is done externally. 👍

@webknjaz
Copy link
Member

webknjaz commented Jun 24, 2024

IIUC we build the package using the action, to fix this we would need to build the package directly without using the action at all?

@nicoddemus you could build the package but not use attestation. Then, you could attest in a separate, isolated job. In my guide @ https://packaging.python.org/en/latest/guides/publishing-package-distribution-releases-using-github-actions-ci-cd-workflows/#signing-the-distribution-packages, there's an example of putting signatures into GH Releases like that — it was written before GH attestations were a thing so using their action instead would work.

There's also work happening over in my pypi-publish action to enable it to generate attestations automatically. Those will also be uploaded to PyPI, once implemented. The Twine CLI arg for that is already present, the PyPI endpoint is probably still a no-op, we're also discussing making that compatible w/ the GH attestations feature. One question I raised there, though, is that it'd sign artifacts downloaded from elsewhere, so it's not really build provenance but upload provenance. That's why, having a dedicated job working with the built artifacts might make sense (pypa/gh-action-pypi-publish#236 (comment)).

The umbrella issue for that effort is pypi/warehouse#15871, by the way.

@nicoddemus
Copy link
Member Author

Cool, thanks for the detailed explanation!

@nicoddemus
Copy link
Member Author

I'm OK with us disabling provenance for now and wait for the pypi-publish work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants