Skip to content

workflows: add trusted publishing release workflow#231

Merged
alex merged 9 commits intocertifi:masterfrom
trail-of-forks:ww/trusted-publishing
Jul 26, 2023
Merged

workflows: add trusted publishing release workflow#231
alex merged 9 commits intocertifi:masterfrom
trail-of-forks:ww/trusted-publishing

Conversation

@woodruffw
Copy link
Copy Markdown
Contributor

@woodruffw woodruffw commented Jul 25, 2023

This adds release.yml, which uses PyPI's trusted publishing functionality to publish releases of certifi to PyPI without needing a pre-shared API token.

This needs a few manual considerations before it can be merged:

  • The tag pattern is currently specified as v*.*.*, whereas the current release tag pattern is just *.*.*. My recommendation would be to move future releases onto the v*.*.* pattern, but it's not a blocker for using trusted publishing and I can change the pattern to just *.*.* if you'd prefer.

  • The new release workflow currently specifies a release environment, which I recommend you configure (both on GitHub and on PyPI, when registering the trusted publisher). My recommendation is that this environment include a deployment protection rule that requires manual approval from a certifi maintainer, meaning that even an attacker who compromises the repository will be unable to force a PyPI publish.

  • Separately, I recommend setting up a tag protection rule for the v*.*.* pattern, if you end up going with that. That protection rule is only applicable if you include the v prefix, since tag protection rules use fnmatch(3) syntax.

  • Finally, this needs a corresponding trusted publisher registration on PyPI! It should be something like:

    Repository Owner: certifi
    Repository Name: python-certifi
    Workflow Name: release.yml
    Environment Name: release
    

Signed-off-by: William Woodruff <william@trailofbits.com>
uses: actions/setup-python@61a6322f88396a6271a6ee3565807d608ecaddd1 # v4.7.0

- name: Build distributions
run: python setup.py sdist bdist_wheel
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

NOTE: I copied this directly from the Makefile, but I can't remember off the top of my head whether bdist_wheel requires python -m pip install wheel first. I didn't see it anywhere else in the CI, so I think it's fine?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We don't build or publish in CI, so that doesn't prove anything ;-)

I believe installing wheel manually is required. Is there a reason not to use pip install build for this?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

None that I can think of, will add.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Added; these are currently unpinned but perhaps it makes sense to hash-pin them in a publish-requirements.txt instead.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should we be using https://pypa-build.readthedocs.io/en/stable/ rather than invoking setup.py directly?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yep, that'd be ideal. Want me to roll that in here or do a separate PR?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Include please

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done; I took the liberty of also removing the setup.py publish patch in a separate commit (since we should never go through setup.py directly again), but I can revert that if you'd prefer.

@alex
Copy link
Copy Markdown
Member

alex commented Jul 25, 2023

I don't have an opinion on the v prefix on tags. Do you @Lukasa?

Otherwise this look ok and I can set up the various protections.

Signed-off-by: William Woodruff <william@trailofbits.com>
Copy link
Copy Markdown
Member

@alex alex left a comment

Choose a reason for hiding this comment

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

Can we add a workflow_dispatch trigger + dry run mode (that does everything except actually upload)?

woodruffw and others added 2 commits July 25, 2023 22:30
Co-authored-by: Alex Gaynor <alex.gaynor@gmail.com>
Signed-off-by: William Woodruff <william@trailofbits.com>
@woodruffw
Copy link
Copy Markdown
Contributor Author

Can we add a workflow_dispatch trigger + dry run mode (that does everything except actually upload)?

Done with 83d428d; let me know if that's what you were thinking (the workflow_dispatch is intrinsically dry run and can never publish).

Signed-off-by: William Woodruff <william@trailofbits.com>
Signed-off-by: William Woodruff <william@trailofbits.com>
Invoking `setup.py` directly is discouraged, and the behavior
in this hacked subcommand is covered by the Makefile.

Signed-off-by: William Woodruff <william@trailofbits.com>
Signed-off-by: William Woodruff <william@trailofbits.com>
Signed-off-by: William Woodruff <william@trailofbits.com>
@alex alex merged commit 0399a7c into certifi:master Jul 26, 2023
@woodruffw woodruffw deleted the ww/trusted-publishing branch July 26, 2023 03:08
@woodruffw woodruffw mentioned this pull request Jul 26, 2023
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.

2 participants