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

Fix broad build privileges @ GHA release workflow #3281

Merged
merged 3 commits into from
May 15, 2024

Conversation

webknjaz
Copy link
Contributor

This makes sure the GHA workflow follows the principle of the least privilege, preventing possible future privilege escalation attacks through poisoning the build dependencies. While the level of privilege compromise suggested already means the compromise of PyPI, the attack surface here is any other services that make use of OIDC and may be connected separately to the repository without considering this workflow, at some point.
So this patch follows the suggestions outlined in https://packaging.python.org/en/latest/guides/publishing-package-distribution-releases-using-github-actions-ci-cd-workflows/ to separate privileges through standalone jobs with individual privileges.
It also makes the URLs rendered in the UI more accurate. And while on it, I simplified the build command into the form that is closer to what pip does — making pypa/build create an sdist from Git but wheel from that sdist. Previously, both were made from Git.

@webknjaz webknjaz requested a review from gaborbernat as a code owner May 13, 2024 22:50
Copy link
Member

@gaborbernat gaborbernat left a comment

Choose a reason for hiding this comment

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

We're making the release slower for almost no gain? What benefit this brings? It Pypi is compromised will upload a compromised release just as well...

@webknjaz
Copy link
Contributor Author

That's what I was trying to explain — the current state allows compromising systems beyond PyPI.

@gaborbernat
Copy link
Member

I don't see how 😅

@webknjaz
Copy link
Contributor Author

If you integrate another service relying on OIDC in the future.

@gaborbernat
Copy link
Member

That's unlikely and we can do that change then 😂

@webknjaz
Copy link
Contributor Author

The idea is that you won't remember by then so defaulting to secure practices is the only way to prevent such a situation.

@gaborbernat
Copy link
Member

I tend to disagree with that statement, I don't know any other systems I'd ever want to integrate with, so I call Yagni.

@webknjaz
Copy link
Contributor Author

I think that AWS is one example.

@gaborbernat
Copy link
Member

I see no reason why I would ever want to pull in AWS 🤔

@webknjaz
Copy link
Contributor Author

I'm just giving an example of what can be compromised. It's up to you to decide if you want the unsafe practices to remain in the repo 🤷‍♂️

@gaborbernat
Copy link
Member

gaborbernat commented May 15, 2024

I think this makes the code more complicated and slow, for no immediate benefit and some potential future case, that well could be YAGNI. So I am tempted to thank for the PR but say not for now. This does not make the repo any more secure until that imaginary future extra integration happens; which most likely will never happen.

@webknjaz
Copy link
Contributor Author

I agree that it's slower but YAGNI does not apply to security the same way as to the other aspects. There are other reasons for the separation that I didn't mention but security seems like the most important one.

@gaborbernat
Copy link
Member

I would be more in favor if it would solve some existing security issue today, but from what we gathered, this today is +0 change when it comes to security of this pipeline. Hence, why digging if there's any benefit today.

@webknjaz
Copy link
Contributor Author

I think being proactive in such matters is something that is useful right away.

FWIW this is recommended by William and me in all the guides/examples we maintain. Currently, also working on fixing these bits in GitHub's own docs and starter templates. So it would just be following a common way of doing this.

@gaborbernat gaborbernat merged commit f4e257c into tox-dev:main May 15, 2024
25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants