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

Improving the security of typing-extensions #393

Closed
JelleZijlstra opened this issue May 15, 2024 · 8 comments · Fixed by #403
Closed

Improving the security of typing-extensions #393

JelleZijlstra opened this issue May 15, 2024 · 8 comments · Fixed by #403

Comments

@JelleZijlstra
Copy link
Member

typing-extensions is among the most widely used Python packages, and we should make sure we keep it secure.

I'd like to propose improvements in a few areas:

Better release process

The current release process involves the release manager (in practice, me) building an sdist and wheel on their laptop and uploading it to PyPI. This opens up a few possible issues: the release manager's laptop could be compromised; they could make a mistake in what files to upload; they could sneakily manipulate the contents of the files before they are uploaded.

We should instead use https://docs.pypi.org/trusted-publishers/ to publish directly from a GitHub Action to PyPI. This removes the release manager's machine from the loop and ensures the release matches what is in the repo. (Recall that the recent xz backdoor involved a tarball with contents different from the repo.)

Manage who has access

To reduce the risk of account compromise, we should limit access to the repo. I would like to propose:

  • Remove commit (and PyPI maintainer) access from those who have not used it recently
  • Ensure that every permission level (e.g., admin) has at least two people in it, so we don't rely on a single person who might disappear or lose interest

Ensure code is reviewed

We should ensure all code in the repo is reviewed. I would like to propose:

  • Require reviews for all pull requests
  • Require pull requests for all changes (since PRs have more visibility than direct commits to the repo)

Keep the repo simple

The less surface area, the less the chance of issues. We should commit to:

  • Never add any non-stdlib dependencies to this repo
  • Never add any non-Python code (e.g., C extensions)

I am happy to implement all of these if other maintainers agree.

@AlexWaygood
Copy link
Member

AlexWaygood commented May 15, 2024

Overall this all sounds great; thanks for writing it up!

the release manager (in practice, me)

FWIW, I'd be happy to help out with releases :-)

We should ensure all code in the repo is reviewed. I would like to propose:

* Require reviews for all pull requests

This is the only part I have a slight qualm about. I feel like PRs like #386 which only touch docs files probably don't really require review. But PRs like that are rare enough that I think it'll probably be okay; I don't have a strong objection here.

@hauntsaninja
Copy link
Collaborator

hauntsaninja commented May 15, 2024

I think all of these are good. Note that requiring PR reviews does not meaningfully improve security posture (to be robust to single adversarial maintainer, you'd need to at least a) enforce previous approvals are invalidated by new commits, b) contributions by external contributors will need double approval).

I'm happy to both help out more with releases or also lose my release bit on PyPI, whatever we feel is best.

@JelleZijlstra
Copy link
Member Author

Thanks both!

I feel like PRs like #386 which only touch docs files probably don't really require review.

I agree reviews aren't terribly important for docs-only changes, but there's still a benefit in having someone else take a look.

Note that requiring PR reviews does not meaningfully improve security posture (to be robust to single adversarial maintainer, you'd need to at least a) enforce previous approvals are invalidated by new commits, b) contributions by external contributors will need double approval).

I just enabled a setting that requires PRs and requires at least one review. GitHub also has a setting that requires review on the last pushed commit, which fixes your point b). It does not fix point a). We could fix that by requiring two (or more) reviews per PR, but I feel that is too inconvenient for development. (Any maintainer PR would need review from two other maintainers.)

@JukkaL
Copy link

JukkaL commented May 15, 2024

The changes look good, and I'm happy to get less access (or no special access), unless a backup maintainer is useful.

@gvanrossum
Copy link
Member

FWIW, on PyPI, we now have three admins (owners) and (in addition) three maintainers. Owners can do everything that maintainers can do. I'm okay with this setup. Not sure what the GitHub setup is yet.

@JelleZijlstra
Copy link
Member Author

You can see who has commit access at https://github.com/python/typing_extensions/settings/access (if you are a maintainer). There's a few people there that we should probably retire.

@AlexWaygood
Copy link
Member

You can see who has commit access at python/typing_extensions/settings/access (if you are a maintainer).

(I think only repo admins can see that page; I'm a maintainer and it's a 404 for me)

@JelleZijlstra
Copy link
Member Author

Status:

  • We now have a Trusted Publishers workflow that successfully released the new release https://pypi.org/project/typing-extensions/4.12.0rc1/. The release process requires manual approval from one of a small set of trusted maintainers.
  • We removed access from a few people who are no longer active.
  • We turned on GitHub settings to require code review on all PRs.
  • Add security documentation #403 adds a discussion of security to our docs.

We should also add a SECURITY.md file.

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 a pull request may close this issue.

5 participants