Automatically refresh short-lived PyPI token in long Trusted Publishing uploads

[facutuesca]

Is there an existing issue for this?

• I have searched the existing issues (open and closed), and could not find an existing issue

What keywords did you use to search existing issues?

trusted publish
oidc
timeout

Please describe the problem you are attempting to solve with this request

When uploading several large distributions to an index, depending on their size and network conditions, uploads might take more than 15 mins.

In Trusted Publishing flows where the OIDC token is exchanged for a PyPI token that is short-lived (15 mins), twine upload will fail between file uploads if the 15 mins are up and there are still files left to upload.

There's at least one case of someone hitting this issue (see here).

How do you think we should solve this?

A possible solution would be for twine to automatically request a new PyPI token between file uploads, if more than 15 minutes have passed since the original token was requested.

Anything else you'd like to mention?

cc @woodruffw @webknjaz

[woodruffw]

Thanks @facutuesca!

For context/cross-referencing: this came up recently at PyCon because of an initial attempt to switch gh-action-pypi-publish to using twine for the Trusted Publishing exchange directly: pypa/gh-action-pypi-publish#360

[sigmavirus24]

I think I see some easy refactoring for this but I'm on a phone so not able to describe it succinctly. When I get to a keyboard I'll write it up better

[webknjaz]

(more context)

There's at least one case of someone hitting this issue (see here).

That time, it was happening while PyPI had network issues or something like that. So the likelihood of more projects hitting this increases when their uploads are large and PyPI has instability.

A possible solution would be for twine to automatically request a new PyPI token between file uploads, if more than 15 minutes have passed since the original token was requested.

I'd say 15 - 1 = 14 minutes to allow for possible time drift in between envs/servers. This is a standard practice in the GitHub Apps ecosystem nowadays, for example.

[sigmavirus24]

So today, we have authentication resolution via Settings. This, however, only ever gets a single token. So with the current design we'd need to reinitialize this over and over again somehow, despite not having that kind of reach. The thing we do have is the ability to (from Settings) create the Repository we're uploading to.

With this in mind, I would argue that we change the following:

• Repository should not take username and password (or if it does, it's mutually exclusive with the new parameter) but instead it takes auth (or auth_helper) or whatever we want to call it
• Resolver should be rethought entirely to have a resolve method which returns an object that implements the Custom Authentication required for requests.
  • If we're doing basic auth, just return requests.auth.HTTPBasicAuth we can either use that directly with static credentials or we can make it dynamic and cache it for some period of time.
  • If we're using trusted publishing, we should return a different class that caches the oidc token and checks the expiry. Personally, I'd start attempting to replace the token earlier than 1 minute prior to expiry, e.g., tokens last 15 minutes, at 5 minutes remaining I'd attempt to replace it. If we can't replace it, we return the non-expired token. On next use, we attempt again, etc.
• The value of Resolver#resolve is now passed into Repository.

In other words, a lot of what's on Resolver today is broken out into other classes, Resolver ends up with just a resolve method which returns something for Requests to use for auth, and Repository takes that auth instance instead of username/pw.

--

Technically, twine.repository, twine.settings, etc. were intended to be part of a public API but we never got buy-in from the folks demanding one (because they just wanted to call twine.upload without giving any cares as to where that goes) so I feel zero qualms about changing this. We never made it the official public API, so if someone's using it and broken by it, I don't care. We can release it as 7.0 if needed and drop some Python versions too if we haven't done that recently enough.

[webknjaz]

An early multi-attempt token swap idea is interesting. Although, it'd complicate the initial solution.

For reference, here are some places where I've seen the expiration offset being used:

• https://github.com/python-trio/snekomatic/blob/43e8e6d/snekomatic/gh.py#L89-L93
• https://github.com/PyGithub/PyGithub/blob/d90323f/github/Consts.py#L172-L174

(I think I also saw this recommendation in some GitHub's docs for Apps)

[sigmavirus24]

Having worked on similar projects we'll likely still see issues if we're not a little more aggressive in trying to exchange the token

[webknjaz]

Agreed. It makes sense for the client side. I'm only worried that the PyPI side might start throttling such requests. We'll have to check in with @miketheman to make sure that we won't cause some unexpected behavior because of the token refresh frequency increase. I think he might've mentioned that each of the requests would generate a security event while we were brainstorming the official PyPI-side support of uploads from reusable workflows last week at PyCon US.

[woodruffw]

I think he might've mentioned that each of the requests would generate a security event while we were brainstorming the official PyPI-side support of uploads from reusable workflows last week at PyCon US.

I think that was me, unless @miketheman had the same concern 🙂 -- the observation there was that PyPI creates a project-level security event for every token exchange (so that maintainers can audit it later), which in turn means that we need to be a little judicious about not exchanging too frequently/aggressively. Basically I was worried that exchanging for each distribution upload would produce too much noise, versus only exchanging when we get close to the current token's expiry (minus some leeway).

In practice, exchanging with 1-5 minutes of leeway (i.e. up to 5 minutes before the token actually expires) would be perfectly fine!

(In terms of throttling: PyPI doesn't currently throttle the exchange itself at all except for in the first-use "pending" publisher case, which shouldn't cause any issues here.)

[sigmavirus24]

In the past I have avoided the complexity of relying on something like threading to parallelize uploads. Would it potentially be a helpful mitigation here too? It will certainly mess up our progress bars but I can imagine that combined with the token refresh logic would help these users regardless

[webknjaz]

@woodruffw yeah, that might've been you. My brain is no longer capable of matching things I've heard over the past two weeks with humans :)

@sigmavirus24 yes, I've also been thinking of threading in this context. Regarding the progress bars, it's best to have them disabled in CI anyway (and we do that in the action automatically).

[sigmavirus24]

Ah yeah, I was just thinking that we always enable threading and so outside of CI this would be bad but ideally fewer and fewer people are running twine locally and caring about the UI

[woodruffw]

Yeah, I like the idea of threading as a mitigation here -- it might even be "good enough" to avoid needing a deeper "refresh" mechanism, since multiple uploads can start at the same time and run at their own paces to avoid having to authenticate with PyPI outside of the short-lived credential window. But that's also not especially sound, so take that idea with a grain of salt.

(Re: progress bars: I think rich's progressbar APIs do support updates from multiple threads, so users outside of CI could probably end up with a halfway-reasonable progress bar render here.)