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

PEP 763: Limiting deletions on PyPI #4080

Merged
merged 36 commits into from
Oct 28, 2024

Conversation

woodruffw
Copy link
Contributor

@woodruffw woodruffw commented Oct 24, 2024

Pre-PEP thread: https://discuss.python.org/t/pre-pep-limiting-deletions-on-pypi/66351

Once allocated a number, I'll create the official PEP discussion thread as well 🙂

Basic requirements (all PEP Types)

  • Read and followed PEP 1 & PEP 12
  • File created from the latest PEP template
  • PEP has next available number, & set in filename (pep-NNNN.rst), PR title (PEP 123: <Title of PEP>) and PEP header
  • Title clearly, accurately and concisely describes the content in 79 characters or less
  • Core dev/PEP editor listed as Author or Sponsor, and formally confirmed their approval
  • Author, Status (Draft), Type and Created headers filled out correctly
  • PEP-Delegate, Topic, Requires and Replaces headers completed if appropriate
  • Required sections included
    • Abstract (first section)
    • Copyright (last section; exact wording from template required)
  • Code is well-formatted (PEP 7/PEP 8) and is in code blocks, with the right lexer names if non-Python
  • PEP builds with no warnings, pre-commit checks pass and content displays as intended in the rendered HTML
  • Authors/sponsor added to .github/CODEOWNERS for the PEP

Standards Track requirements

  • PEP topic discussed in a suitable venue with general agreement that a PEP is appropriate
  • Suggested sections included (unless not applicable)
    • Motivation
    • Rationale
    • Specification
    • Backwards Compatibility
    • Security Implications
    • How to Teach This
    • Reference Implementation
    • Rejected Ideas
    • Open Issues
  • Python-Version set to valid (pre-beta) future Python version, if relevant
  • Any project stated in the PEP as supporting/endorsing/benefiting from the PEP formally confirmed such
  • Right before or after initial merging, PEP discussion thread created and linked to in Discussions-To and Post-History

Signed-off-by: William Woodruff [email protected]


📚 Documentation preview 📚: https://pep-previews--4080.org.readthedocs.build/pep-0763/

@woodruffw woodruffw requested a review from a team as a code owner October 24, 2024 16:49
Copy link

cpython-cla-bot bot commented Oct 24, 2024

All commit authors signed the Contributor License Agreement.
CLA signed

@woodruffw woodruffw changed the title drafting deletion PEP PEP: Limiting deletions on PyPI Oct 24, 2024
peps/pep-9999.rst Outdated Show resolved Hide resolved
@AA-Turner AA-Turner added the new-pep A new draft PEP submitted for initial review label Oct 24, 2024
@AA-Turner AA-Turner changed the title PEP: Limiting deletions on PyPI PEP 763: Limiting deletions on PyPI Oct 24, 2024
peps/pep-9999.rst Outdated Show resolved Hide resolved
@hugovk
Copy link
Member

hugovk commented Oct 24, 2024

  • Core dev/PEP editor listed as Author or Sponsor, and formally confirmed their approval

@dstufft Please could you confirm your sponsorship of this PEP?

woodruffw and others added 4 commits October 24, 2024 15:50
Co-authored-by: Adam Turner <[email protected]>
Signed-off-by: William Woodruff <[email protected]>
Signed-off-by: William Woodruff <[email protected]>
Signed-off-by: William Woodruff <[email protected]>
Copy link
Member

@AA-Turner AA-Turner left a comment

Choose a reason for hiding this comment

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

The build is failing because this PR is a bit out of date -- but we can't use the "Update branch" button because this PR is made from a fork. If you merge in the lastest HEAD then I think it should pass (and you'll fill in the missing numbers in CODEOWNERS).

Other than that, a few comments in-line.

A

peps/pep-0763.rst Outdated Show resolved Hide resolved
peps/pep-0763.rst Outdated Show resolved Hide resolved
peps/pep-0763.rst Outdated Show resolved Hide resolved
peps/pep-0763.rst Outdated Show resolved Hide resolved
peps/pep-0763.rst Outdated Show resolved Hide resolved
peps/pep-0763.rst Outdated Show resolved Hide resolved
Comment on lines 182 to 183
This PEP does not identify any positive or negative security implications
associated with proposed approach.
Copy link
Member

Choose a reason for hiding this comment

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

Given above we discuss security implications of permitting deletions, I would expect that the security implications are this PEP makes things better (but also see my comment above).

Copy link
Member

Choose a reason for hiding this comment

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

A possible negative implication is that it may be harder to stop users from downloading a file that has security issues, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! Yeah, I need to qualify this: the positive implication above was that external users of the index (e.g. companies) benefit from limiting deletion because it makes post-compromise triage and remediation simpler, since the attacker can't remove their trail as easily. I wasn't sure whether to put that under this section though, since it's an indirect/fuzzy security implication, but I'll work it in for further discussion.

A possible negative implication is that it may be harder to stop users from downloading a file that has security issues, right?

@JelleZijlstra Could you elaborate a bit on what you're thinking here? This PEP doesn't stipulate any changes to PyPI admins' abilities to delete things, so the existing malware reporting and quarantining flows remain in place. So from my perspective this doesn't make it any harder to halt downloads of malicious packages.

OTOH, maybe you're thinking of packages that aren't malicious but just have security issues? For those I think the pre-existing "yank" mechanism is good enough (and is what people already use in practice), but I could add in some qualifications there if you think the PEP would benefit from it!

Copy link
Member

Choose a reason for hiding this comment

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

Currently, if a package maintainer discovers that there is malware in a package they released, they can delete the package. With this PEP's proposal, they'd have to instead wait for PyPI admins to get to it, which may take longer.

Example scenario: I am a maintainer of https://pypi.org/project/black/. I discover (e.g., through private disclosure) that one of the compiled wheels of the latest release contains malicious code due to a backdoor in cibuildwheel/GH Actions/mypyc (pretty outlandish scenarios, but bear with me). Right now, the first thing I would do is go to PyPI and delete the malicious wheel. With this PEP's proposal, I'd have to figure out the right channels to raise the issue to the PyPI admins, and possibly wait for some time.

This is not a common scenario and your answer could well be that the tradeoff is worth it, but it doesn't seem impossible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Gotcha, thanks for elaborating! That's an interesting case -- I can see the argument that it represents a slightly worse security posture, since there's now a middleman (PyPI's admins) for malware response.

I think my position is that it's "worth it," especially now that PyPI has a dedicated malware reporting flow (see screencap) as well as project "quarantine" support for temporarily suspending a project's downloads without actually deleting it. But I'll make sure to include that in a revision to fairly characterize this PEP's changes.

Screenshot 2024-10-28 at 12 10 53 PM

Copy link
Member

Choose a reason for hiding this comment

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

Thanks, that position makes sense to me!

peps/pep-0763.rst Outdated Show resolved Hide resolved
peps/pep-0763.rst Outdated Show resolved Hide resolved
peps/pep-0763.rst Outdated Show resolved Hide resolved
peps/pep-0763.rst Outdated Show resolved Hide resolved
peps/pep-0763.rst Outdated Show resolved Hide resolved
Comment on lines +45 to +47
This leaves projects in a catch-22 situation where new projects may be pulling
down this known broken version, but if they do anything to prevent that they’ll
break projects that are already using it.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
This leaves projects in a catch-22 situation where new projects may be pulling
down this known broken version, but if they do anything to prevent that they’ll
break projects that are already using it.
This leaves a project author in a catch-22 situation where other projects may be pulling
down this known broken version, but if the project author does anything to prevent that, it
breaks projects where the dependency has been pinned for use.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as https://github.com/python/peps/pull/4080/files#r1817037637 -- I like this rephrasing but I don't know whether it's kosher to rewrite a direct quote 😅

peps/pep-0763.rst Outdated Show resolved Hide resolved
peps/pep-0763.rst Outdated Show resolved Hide resolved
peps/pep-0763.rst Outdated Show resolved Hide resolved
peps/pep-0763.rst Outdated Show resolved Hide resolved
peps/pep-0763.rst Outdated Show resolved Hide resolved
peps/pep-0763.rst Outdated Show resolved Hide resolved
peps/pep-0763.rst Outdated Show resolved Hide resolved
peps/pep-0763.rst Outdated Show resolved Hide resolved
peps/pep-0763.rst Outdated Show resolved Hide resolved
peps/pep-0763.rst Outdated Show resolved Hide resolved
peps/pep-0763.rst Outdated Show resolved Hide resolved
.github/CODEOWNERS Outdated Show resolved Hide resolved
@hugovk
Copy link
Member

hugovk commented Oct 25, 2024

Once allocated a number, I'll create the official PEP discussion thread as well 🙂

Please do this after the PR is merged :)

peps/pep-0763.rst Outdated Show resolved Hide resolved
peps/pep-0763.rst Outdated Show resolved Hide resolved
Comment on lines 182 to 183
This PEP does not identify any positive or negative security implications
associated with proposed approach.
Copy link
Member

Choose a reason for hiding this comment

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

A possible negative implication is that it may be harder to stop users from downloading a file that has security issues, right?

@JelleZijlstra
Copy link
Member

I think the PEP could be make stronger with a discussion of what other ecosystems do. There's some discussion of NPM, but do any other packaging systems (maybe Rust, Go, OPAM, Cabal, ...) allow deletions? If so, under what circumstances? Is there any precedent for not allowing deletions?

Signed-off-by: William Woodruff <[email protected]>
These are not strictly relevant to the PEP itself.

Signed-off-by: William Woodruff <[email protected]>
@dstufft
Copy link
Member

dstufft commented Oct 28, 2024

@dstufft Please could you confirm your sponsorship of this PEP?

Yes.

woodruffw and others added 9 commits October 28, 2024 10:02
Signed-off-by: William Woodruff <[email protected]>
Signed-off-by: William Woodruff <[email protected]>
Co-authored-by: Jelle Zijlstra <[email protected]>
Co-authored-by: Hugo van Kemenade <[email protected]>
Co-authored-by: Hugo van Kemenade <[email protected]>
Signed-off-by: William Woodruff <[email protected]>
Signed-off-by: William Woodruff <[email protected]>
Signed-off-by: William Woodruff <[email protected]>
@woodruffw
Copy link
Contributor Author

I think this is ready for merge, so that I can make the discussion thread! I don't think I covered 100% of the topics raised above, but I'd like to punt that to discussions so that the conversation isn't siloed here 😅

(In particular, I'm flagging @JelleZijlstra's precedent comment as one I need to address -- there's ample precedent in other packaging ecosystems and I'll add that to this PEP in a follow-up.)

peps/pep-0763.rst Outdated Show resolved Hide resolved
Co-authored-by: Hugo van Kemenade <[email protected]>
@JelleZijlstra JelleZijlstra merged commit f404195 into python:main Oct 28, 2024
5 checks passed
@woodruffw woodruffw deleted the ww/deletion-pep branch October 28, 2024 20:24
gvanrossum pushed a commit to gvanrossum/peps that referenced this pull request Dec 10, 2024
Signed-off-by: William Woodruff <[email protected]>
Co-authored-by: Alexis <[email protected]>
Co-authored-by: dm <[email protected]>
Co-authored-by: Adam Turner <[email protected]>
Co-authored-by: Carol Willing <[email protected]>
Co-authored-by: Jelle Zijlstra <[email protected]>
Co-authored-by: Hugo van Kemenade <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new-pep A new draft PEP submitted for initial review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants