Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat(blog): blog post--lessons learned on publish security #538
base: main
Are you sure you want to change the base?
feat(blog): blog post--lessons learned on publish security #538
Changes from 1 commit
b236a94
fb6318f
d85f1a2
572e705
6a1fd45
029bd04
e16facb
4a80188
7cf3279
1bd173c
a285a13
b9b8d3e
d6630bc
3215e26
21bb0cf
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
It's not immediately clear what makes a workflow “release-based”. I haven't read past this point at the time of writing this comment, though.
Rephrase, perhaps?
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.
good point - fixed!!
pull_request_target
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.
That's probably an oversimplification. Not caching the deps may as well lead to something like this if the transitive deps are unpinned and get updated in uncontrolled manner. The problem was that the cache was poisoned in a job with a security context that had high privileges and then, another job picked it up, effectively reusing what that first one “pinned” in the installed deps.
But if there's no cache, a compromised transitive dependency can be downloaded from PyPI, resulting in the same problem.
Security exists in context and not in isolation. It should come from a place of understanding how different bits fit together.
That said, I wouldn't make a call of whether using caching or not will save us all. The problem was that it was possible to compromise the cache that ended up being used.
I think it's possible to use cache securely by having separate cache keys unique to the dist building job. So caches for building the dists and testing them would not leak into each other. That's the real fix for cache.
Another side of this is the supply chain problem, which cache ended up being a part of in this instance. But as I mentioned earlier, the supply chain could be compromised differently, without cache being a factor. The solution to this is pinning your dependencies. This means recording every single version number for every single transitive dependency there is. One of the low-level tools to use for this is
pip-tools
, in the context ofpip install
(until we have PEP 751, that is). Non-pip installers have different mechanisms. With pip, you can usepip install -r requirements.in -c constraints.txt
, whererequirements.in
would contain a direct and unrestricted dependency list, andconstraints.txt
would set the entire tree to exact versions. A complete solution is usually integration of different aspects through multiple tools (or a "one-tool-to-rule-them-all" which some projects aim to become). I've been taking this to extremes with "DIY lock files" in https://github.com/webknjaz/ep2024-multilock-toy-template / https://github.com/ansible/awx-plugins/tree/2c2b22c/dependencies/lock-files.Additionally, there's
--require-hashes
which also records every single dist file hash, not just version (this is not compatible with-c
, though, due to a bug in pip).To summarize, the solution here is to be in control of your deps, make sure their updates are transparent and controllable on all the levels, and to know what influences them… Out of the context, disallowing caching is a band-aid at best. But it may be harmful in that it would cultivate a false sense of security.
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.
ok. This is super helpful. I could remove the caching part altogether, given so many moving pieces and caching seems more nuanced then just locking down the workflow itself with better triggers and tighter permissions. But I also have a question about the dependencies. Because it is my understanding and experience that pinning all deps can also be highly problematic for users. For instance, if you have an environment with many different packages and try installing a package with pinned dips, that could lead to significant user issues.
Unless we're just talking about dips in the workflow itself, such as hatch, pypa/build, etc.
web apps make sense to pin but i can't quite wrap my head around pinning deps for a package. Could you please help me understand? And do you agree - maybe for more beginner users we just drop caching altogether as a topic?
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.
While the principle of the least privilege is good to follow, this specific breach wasn't because somebody had direct access to the repo (the attack wouldn't need to be as sophisticated), but because the security context used for PRs from arbitrary forks was set to trusted w/o performing the necessary precautions.
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 this is mostly about setting an environment in the job and making sure that environment has required reviewers in the repo settings.
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.
Looking at the blog posts, it seemed like the hackers somehow got direct access to that repo. Did i read that wrong?
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.
2FA is already required for any uploads to PyPI since this year. There's no way around anymore. That said, long-living and over-scoped API keys may leak and should be rotated periodically. It's much easier to use Trusted Publishing instead — it uses short-lived tokens every time, they expire in 15 minutes upon creation and one doesn't need to do a thing to revoke them.
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.
Ok, I hear you. I've added this:
I'll likely remove 2FA altogether, given it's required. I doubt most users understand why making tokens infinitely valid is bad!
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.
You meant
pypi-publish
, right? Or did you mean GitHub Actions CI/CD as an ecosystem? Or a GitHub Actions Workflow?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.
here, I'm referring to GitHub actions in general but our examples do use the pypa action . But you are making a good point.
i'll come back to this comment and will make sure that point is super clear in the text.
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 is not tested and could be wrong.
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 is one of the uses but oftentimes, building is performed by
pypa/build
so it'd be justpip install build
and not multiple direct deps as one might expect from arequirements.txt
file. So the example seems a bit unrealistic, except for some untypical projects with surprising build automation required.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.
oh, right, good point. What is a good example of caching dependencies (other than what we say with ultranalytics)? I use caching when i'm publishing.
we pip install hatch and use
hatch build
What do you think about removing the example and just telling people that if they do cache dependencies for any reason, don't do it when the build is for publishing?
for instance, doing it for a website build could be ok if it's a preview, etc. vs publishing a package?
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 example shows installing some abstract deps but does not demonstrate using them (as in, running
python -Im 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.
NEVER EVER build in the same job as publishing. This is an insecure practice. I had to recently spell it out @ https://github.com/marketplace/actions/pypi-publish#Non-goals.
One of the reasons (probably the most important one in the context of this document) is that Trusted Publishing requires one to increase privileges of the publishing job, giving it access to OIDC. This privilege must not be shared with transitive build dependencies. Such a privilege escalation may allow a malicious actor to impersonate your workflow in systems beyond PyPI (but also increases the poisoning surface for the dists published on PyPI).
Ultimately, the only workflow shape I'm willing to call supported is documented @ https://packaging.python.org/en/latest/guides/publishing-package-distribution-releases-using-github-actions-ci-cd-workflows/.
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 will add a link to that packaging guide. And i see why this example may direct users incorrectly. in fact, i remember you telling me this last year when we were working on pyosMeta to update that 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.
IIRC @woodruffw was against claiming that TP is more secure, so we settled on saying that it's “as secure” but is more convenient in many regards in the README of
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.
oh. that's different from my understanding of trusted publishing. I thought that TP provides a secure "pipeline" between the workflow and PyPI vs. a token that I suppose provides access to PyPi, but it doesn't necessarily directly connect to the workflow (i.e., you can specify a specific environment and workflow file with a trusted publisher.
This statement challenges my understanding of trusted publisher. @woodruffw IF you happen to have a moment to clarify that would be super helpful.
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.
TP is more secure in that the tokens are short-lived compared to perpetual API tokens.
And it seems like the hackers got hold of forgotten API tokens in the repo to be able to make the second pair of releases in this case.
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.
Personally, I like calling it a “trust link”. Perhaps, @woodruffw would have opinions on the wordings/metaphors here.
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 like the pipe idea as it's a visual people can understand even if it's not perfect. An isolated pipe or channel where files (sdist, wheel) can "travel" and be verified between GitHub and PyPI. 🤷🏻♀️ maybe there is a better visual but i want to help people understand why this is a good way to design a build / publish workflow.
I want to create a graphic to help people understand this, as I think some of these important concepts get lost because it's all so technical!
But i say that knowing that you and others are experts - i'm just trying to simplify so more people can apply some of these things!
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 builds, but uploads. We make “publish” attestations, as the action cannot know how the dist it sends over was actually built. It should be built in a separate job. But theoretically could be built elsewhere and downloaded from S3 or wherever.
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.
Fixed!
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.
Technically, the environment is attached to a job in a workflow. This is the information PyPI would receive when looking up the trust link. Environments, however, can also be additionally configured in repository settings on GitHub. The recommended environment name is
pypi
. And the recommended configuration is having required reviewers (which would pause said job on GH, requiring a human to click the button).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.
@webknjaz I have not set this up myself and just noticed this feature after reading your comment. thank you. I really don't think people know about this stuff.
TODO: I'll add a small note about how to do that in the post with a screenshot of what the interface looks like.
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 don't fully agree with that note on unauthorized publication. Unauthorized publication can be prevented by controlling the trigger of the release process. Or setting up required reviewers in the environment used in the publishing job (usually called
pypi
) — this would prevent said job from even starting until a trusted human being clicks a button on GH. Recording the environment name on the PyPI side, though, allows restricting the “identity” of the workflow+repo+env that is to be considered “trusted”. But this only works on the authentication level between PyPI and GH. But not between GH and human, which is what you also want to secure.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.
ok, I am still working through your comments, but after I plan to reread the entire thing, and i'll reframe it. concepts:
People will forget the step of updating their token to be project-level scope. So a trusted publisher, in my mind, is less risky for a user because it's simpler.
maybe there is a graphic here to simplify and help people understand what you wrote above?
gh + human authorization
github + PyPI authorization
These are two different but related and important concepts. And they are high-level. like if people need to remember 2 things - these are the things.
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.
cc @woodruffw apparently we forgot to update the screenshots in the warehouse doc to show
pypi
as the environment name.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.
Oops! I can open an issue if that is helpful in warehouse?
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.
Maybe, don't mention that the environment is optional if this post is focused on maximizing security.
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.
good point!! changed to (they will see optional in the image!)
environment (STRONGLY recommended)
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.
In the doc you already linked, there's an example for GitLab as well: https://docs.pypi.org/trusted-publishers/adding-a-publisher/#gitlab-cicd. There are 4 platforms that support this mechanism already, and I saw some Bitbucket folks asking around whether they can integrate.
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.
fixed the breakout and added that link!! I somehow read that GitLab support wasn't available yet, but obviously, I wasn't looking in the right place! thanks!
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 started using the exact repository ID some time ago. This is more accurate and would survive renames and transfers:
(looked up @ https://api.github.com/repos/pyOpenSci/pyopensci.github.io)
If you want to check for the org, you could use the org ID. Org names are mutable and if the org gets renamed for whatever reason (might be a user account that hosts a repo in some cases), this check would break.
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.
oh wow - I've always used the username as the conditional value. The only challenging piece here for users is they'd have to use the API to get that repo id value. I don't see a clear way to get it from the GitHub GUI. we could make a note about an owner name being mutable however and provide the command to find the ID (it requires a token i think?).
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 just stick with the org/username, it's much more readable and transparent.
Org renames/transfers are pretty rare. And you'd notice the problem right away after the first deploy and it's an easy fix.
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.
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.
Add: a few sentences on
delete old tokens and secrets containing pypi tokens... this is easy for people to do and good for them to know about.
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.
Please, replace this screenshot with one having
pypi
typed into the last field.release
is misleading — we've updated it in many places in the PyPI docs already, and on the website form, in the placeholder. But it looks like I missed the screenshots.