-
Notifications
You must be signed in to change notification settings - Fork 58.3k
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
OIDC: add initial PyPI docs #24772
OIDC: add initial PyPI docs #24772
Conversation
Signed-off-by: William Woodruff <william@trailofbits.com>
👋 Hey there spelunker. It looks like you've modified some files that we can't accept as contributions. The complete list of files we can't accept are: You'll need to revert all of the files you changed in that list using GitHub Desktop or |
Automatically generated comment ℹ️This comment is automatically generated and will be overwritten every time changes are committed to this branch. The table contains an overview of files in the Content directory changesYou may find it useful to copy this table into the pull request summary. There you can edit it to share links to important articles or changes and to give a high-level overview of how the changes in your pull request support the overall goals of the pull request.
fpt: Free, Pro, Team |
@woodruffw Thanks for the PR and for linking it to your issue! ✨ As this isn't ready yet, I'm going to triage it for review, but we won't merge this until you're ready. Please ping me again here when it's ready for merge ⚡ |
Actually, would this be better served as a draft PR until it's ready? |
Thanks for triaging @cmwilson21! I think this can be considered ready for review; I see the CI is red but I assume that it's mostly just lints that I need to handle 🙂 |
This comment was marked as outdated.
This comment was marked as outdated.
Signed-off-by: William Woodruff <william@trailofbits.com>
This comment was marked as outdated.
This comment was marked as outdated.
Lintage fixed, but I'm not sure what's up with the link check -- perhaps that's failing because of the warnings about these files not being editable by third-party contributors? The path looks correct to me 🙂 |
@woodruffw does this need the updated "Trusted Publishers" terminology injected in some of the places in these docs? |
Signed-off-by: William Woodruff <william@trailofbits.com>
This comment was marked as outdated.
This comment was marked as outdated.
Yep, I've made those changes -- I've preserved "OIDC" in places where we're talking about the GitHub functionality or IdP specifically, and changed the rest to "trusted publishing." |
@cmwilson21 This should be good for another review! The only failures are in the unallowed files check and the link checker, both of which I believe are happening because I'm not technically allowed to modify these 🙂 CC @MylesBorins and @steiza for factual/style checks as well, since I know you're invested in this landing 🙂 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @woodruffw, some minor comments, otherwise looks great to me!
...actions/deployment/security-hardening-your-deployments/configuring-openid-connect-in-pypi.md
Show resolved
Hide resolved
...actions/deployment/security-hardening-your-deployments/configuring-openid-connect-in-pypi.md
Outdated
Show resolved
Hide resolved
...ployment/security-hardening-your-deployments/about-security-hardening-with-openid-connect.md
Outdated
Show resolved
Hide resolved
...actions/deployment/security-hardening-your-deployments/configuring-openid-connect-in-pypi.md
Show resolved
Hide resolved
This comment was marked as off-topic.
This comment was marked as off-topic.
Co-authored-by: Zach Steindler <steiza@github.com>
This comment was marked as outdated.
This comment was marked as outdated.
...ployment/security-hardening-your-deployments/about-security-hardening-with-openid-connect.md
Outdated
Show resolved
Hide resolved
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
…/configuring-openid-connect-in-pypi.md
This comment was marked as outdated.
This comment was marked as outdated.
Signed-off-by: William Woodruff <william@trailofbits.com>
This comment was marked as outdated.
This comment was marked as outdated.
1 similar comment
👋 Hey there spelunker. It looks like you've modified some files that we can't accept as contributions. The complete list of files we can't accept are: You'll need to revert all of the files you changed in that list using GitHub Desktop or |
Co-authored-by: Joe Clark <31087804+jc-clark@users.noreply.github.com>
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
Signed-off-by: William Woodruff <william@trailofbits.com>
This comment was marked as outdated.
This comment was marked as outdated.
Is there any way we can suppress these bot comments? This PR intentionally modifies files (due to a request from GH for docs) that would otherwise be considered out of scope for a third-party contribution. |
...actions/deployment/security-hardening-your-deployments/configuring-openid-connect-in-pypi.md
Outdated
Show resolved
Hide resolved
...actions/deployment/security-hardening-your-deployments/configuring-openid-connect-in-pypi.md
Outdated
Show resolved
Hide resolved
👋 Hey there spelunker. It looks like you've modified some files that we can't accept as contributions. The complete list of files we can't accept are: You'll need to revert all of the files you changed in that list using GitHub Desktop or |
👋 Hey there spelunker. It looks like you've modified some files that we can't accept as contributions. The complete list of files we can't accept are: You'll need to revert all of the files you changed in that list using GitHub Desktop or |
It looks like this could be merged. However, we can't do this here in this repo due to rules restricting changes to this part of the documentation in the OS repository. I'll create a new PR in our internal docs repo and move these changes over there so that we can get this merged. @woodruffw - Many thanks for contributing to the docs. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hubwriter I've found several improvement possibilities. Would you be open to incorporating them in follow-ups?
Also, we're working on updating the PyPUG guide that might be of interest here: pypa/packaging.python.org#1261.
# NOTE: put your own distribution build steps here. | ||
python -m build | ||
|
||
- name: upload windows dists |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not "windows" dists but Python distribution packages.
name: release-dists | ||
path: dist/ | ||
|
||
pypi-publish: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd add an environment example here.
- Owner: `myorg` | ||
- Repository name: `myproject` | ||
- Workflow name: `release.yml` | ||
- (Optionally) a GitHub Actions environment name: `release` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the release
name is confusing. I used to use it myself in the past but moved away from it in favor of pypi
:
- There could be
testpypi
and other envs too, calling this onepypi
describes the destination accurately. - There could be other targets in separate jobs like publishing to GH releases etc.
- All of above could match the name
release
because it's overly generic. But semantically, those would be different release targets. - Some of the envs would need protection rules while other would be unconditional (like requiring a human approval for PyPI but not TestPyPI).
|
||
```yaml{:copy} | ||
jobs: | ||
release-build: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd probably explicitly drop the privileges in this job or globally and emphasized that building must be done in a locked down environment that is separate from the publishing having elevated privileges.
|
||
To use OIDC with PyPI, add a trust configuration that links each project on PyPI to each repository and workflow combination that's allowed to publish for it. | ||
|
||
1. Log into PyPI and navigate to the trusted publishing settings for the project you'd like to configure. For a project named `myproject`, this will be at `https://pypi.org/manage/project/myproject/settings/publishing/`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could clarify that it shows an example for existing projects but it's possible to achieve the same with fresh ones too.
Thanks @webknjaz. If you could use the code review functionality for suggesting a specific change: to indicate exactly what changes you would like to make to the Markdown I can transfer those suggestions to the new PR I've opened internally. |
Status update: We're handling the changes currently proposed in this PR by @woodruffw in an internal PR now. This excludes any further changes arising from @webknjaz's comments (which I propose we handle with a new docs issue). The relevant product manager requested a couple of SMEs look at this and they have now got back to me. One of the SMEs asked for some further information. I have contacted @woodruffw directly about this. Once I get sign-off from the SME I will merge/publish our internal PR. |
The new article has now been published: Many thanks @woodruffw for contributing this. 🎖️ @webknjaz - If you would like to suggest changes to the article please raise a docs issue: |
This is a work in progress.
Why:
Closes #24594.
What's being changed (if available, include any code snippets, screenshots, or gifs):
This adds some documentation and a guide for OIDC federation between GitHub and PyPI, the Python Package Index. PyPI's OIDC publishing support is currently in a closed beta, so these changes are not 100% ready for public consumption yet; I'm just pushing this up for visibility + to get the ball rolling for when they become generally available 🙂
Check off the following: