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

Uploads: Remove the username=__token__ requirement? #15140

Open
woodruffw opened this issue Jan 4, 2024 · 11 comments
Open

Uploads: Remove the username=__token__ requirement? #15140

woodruffw opened this issue Jan 4, 2024 · 11 comments
Labels
security Security-related issues and pull requests

Comments

@woodruffw
Copy link
Member

Now that 2FA is mandatory, username/password pairs can no longer be used for package upload.

This affords a potential simplification in the upload endpoint's credential format: the phony __token__ username is no longer needed for disambiguation, since all "passwords" are now just API tokens of the form pypi-....

Pros: Delete a small amount of code, remove a shoehorned special value, one less configuration step/variable.

Cons: Probably needs a bunch of doc updates, user benefit is marginal (?)

CCing @miketheman for opinions 🙂

@miketheman miketheman added the security Security-related issues and pull requests label Jan 4, 2024
@miketheman
Copy link
Member

Now that 2FA is mandatory, username/password pairs can no longer be used for package upload.

We still have the BasicAuth security policy in place today, which will inform the user using user/pass about the new approach. It's currently post-validation, so it might be useful to move that all the way up front, and thus remove a lot of that logic.

Considering the dominating client for uploads is twine, it would make most sense to me to have that client evolve to:

  • only ask users to use the token (even to update .pypirc examples to prefer token, no more user/pass)
  • have clients send the token only as a bearer token auth header, so we don't have to extract the basic part (ref)

I think it'll take a significant amount of long-tail time until users have safely migrated to whatever the new approach is.
And yes, definitely need updates to all docs everywhere 🙈

In your Pros, you call out some code reduction - can you poiint out what would be able to go away?

@di
Copy link
Member

di commented Jan 4, 2024

Considering the dominating client for uploads is twine, it would make most sense to me to have that client evolve to:

Note that twine is still used for many third-party package indexes that aren't PyPI -- it likely can't drop support for username/password auth entierly anytime soon, but it could possibly change it's behavior by special-casing PyPI.

@miketheman
Copy link
Member

it likely can't drop support for username/password auth entierly anytime soon, but it could possibly change it's behavior by special-casing PyPI.

I think @woodruffw is all over that already ;) pypa/twine#1040

@woodruffw
Copy link
Member Author

In your Pros, you call out some code reduction - can you poiint out what would be able to go away?

Emphasis on very small 😅 -- I was mostly just thinking about the 1-2 places where username == "__token__" is checked explicitly, and a few of the places where we unmash the basic auth/other credential input formats (if those are still different?). Realistically, this would be <10 lines saved, so this is barely a "pro."

@dstufft
Copy link
Member

dstufft commented Jan 5, 2024

I think that nearly 100% of authentication happens by smuggling the token through basic auth as the password, and the basic auth stuff requires there to be something for username (even if it's the empty string). I guess we could just ignore the username and just let it be anything? Though it seems useful still to register explicit intent.

@woodruffw
Copy link
Member Author

Yep, I believe it'd have to be the empty string in that case. So yeah, this probably currently has limited practical value.

@miketheman
Copy link
Member

miketheman commented Jan 5, 2024

smuggling the token through basic auth as the password,

For reference, here's how it works:

if auth_method == "basic":
return _extract_basic_macaroon(auth)
elif auth_method in ["token", "bearer"]:
return auth

and later, we check the username:

if auth_method != "__token__":
return None

I'd be much happier to see the "basic" part go away, and for folks to use "token" or "bearer" method and skip the need for username extraction.

@woodruffw
Copy link
Member Author

100% agreed -- maybe it'd be good to add stats/counters to those branches, to figure out when it'd be okay to remove the basic auth path?

@dstufft
Copy link
Member

dstufft commented Jan 5, 2024

I suspect nearly 100% of uses currently use the basic auth branch-- afaik it's the only thing Twine supports, and the only thing most upload tools support. There's no standard around the bearer auth thing currently, so upload clients would have to special case PyPI to support it.

@woodruffw
Copy link
Member Author

so upload clients would have to special case PyPI to support it.

I've been recently making other special-case changes to twine (for the PGP deprecation + hardcoded __token__ user on PyPI and TestPyPI), so it probably wouldn't be too hard to add an additional special-case for the auth mechanism. That wouldn't help with the very long tail, though...

@dstufft
Copy link
Member

dstufft commented Jan 5, 2024

I got distracted by non OSS stuff so I hadn't been able to work on it, but I did have python/peps#3172 I started on that would allow handling these cases without special casing them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
security Security-related issues and pull requests
Projects
None yet
Development

No branches or pull requests

4 participants