pypa / gh-action-pypi-publish

The blessed :octocat: GitHub Action, for publishing your :package: distribution files to PyPI, the tokenless way: https://github.com/marketplace/actions/pypi-publish
https://packaging.python.org/guides/publishing-package-distribution-releases-using-github-actions-ci-cd-workflows/
BSD 3-Clause "New" or "Revised" License
928 stars 87 forks source link

invalid-publisher: valid token, but no corresponding publisher part 2 #217

Closed lwasser closed 8 months ago

lwasser commented 8 months ago

Hi there πŸ‘‹ - i did have a look at #173 and it might be relevant to us too? or not ❓ πŸ™ƒ

I seem to be encountering a similar issue with our trusted workflow setup and i could use some guidance in fixing it. I've tried this on both test-pypi and PyPI and have the same outcome. Our package (pyosmeta) is not yet on test pypi but it is on pypi.

here is our workflow

Here is the output from the workflow in this failed run:

Trusted publishing exchange failure:

Token request failed: the server refused the request for the following reasons:

invalid-publisher: valid token, but no corresponding publisher (All lookup strategies exhausted) This generally indicates a trusted publisher configuration error, but could also indicate an internal error on GitHub or PyPI's part.

The claims rendered below are for debugging purposes only. You should not use them to configure a trusted publisher unless they already match your expectations.

If a claim is not present in the claim set, then it is rendered as MISSING.

sub: repo:pyOpenSci/pyosMeta:ref:refs/tags/v0.2.2 repository: pyOpenSci/pyosMeta repository_owner: pyOpenSci repository_owner_id: 28938222 job_workflow_ref: pyOpenSci/pyosMeta/.github/workflows/publish-pypi.yml@refs/tags/v0.2.2 ref: refs/tags/v0.2.2 You're seeing this because the action wasn't given the inputs needed to perform password-based or token-based authentication. If you intended to perform one of those authentication methods instead of trusted publishing, then you should double-check your secret configuration and variable names.

A few notes

The url provided above is slightly different than the actual github location. i am not sure if that has anything to do with the error or not?

I've tried to add the (optional) environment to the workflow (and remove it). I did add the permissions to our workflow file . i didn't have that at first but read it was required in the docs.

permissions:
      id-token: write  # this permission is mandatory for pypi publishing

I do have things setup both on pypi and test pypi image

I am not sure what else to try! i'd love some guidance. unlike #173 this workflow has not yet ever worked for me. many thanks y'all!

### Tasks
webknjaz commented 8 months ago

here is our workflow

That's a workflow with tests, not publishing. I believe you meant to link https://github.com/pyOpenSci/pyosMeta/blob/main/.github/workflows/publish-pypi.yml.

webknjaz commented 8 months ago

An immediate recommendation β€” don't build your dists in the same job as publishing. Exposing the build machinery to the OIDC privilege is dangerous security-wise, as it may lead to privilege escalation in other systems through build dep poisoning. https://packaging.python.org/en/latest/guides/publishing-package-distribution-releases-using-github-actions-ci-cd-workflows/ demonstrates good job separation practices.

webknjaz commented 8 months ago

@lwasser ultimately, your problem is that you configured trust to the correct workflow, on the PyPI side and set a requirement for that workflow to have a GitHub Environment called release. But you didn't set the release environment on the job in the CI/CD workflow definition publish-pypi.yml. This is why, the PyPI responds with an error Β­β€” you told it to only allow publishing from jobs that have the release environment assigned but the one you're attempting to publish from doesn't have any envs set.

That's not a URL, it's a simplified identifier that PyPI shows you. The URL on GitHub is https://github.com/pyOpenSci/pyosMeta/blob/v0.2.2/.github/workflows/publish-pypi.yml.

webknjaz commented 8 months ago

Looking closer into the log @ https://github.com/pyOpenSci/pyosMeta/actions/runs/8058291379, and comparing it with your screenshot, I noticed that your PyPI trust is configured for pyOpenSci/pyosmeta (with a lower-case m) as opposed to pyOpenSci/pyosMeta (upper-case M) that GitHub sends to PyPI. I'm not sure if the PyPI normalizes the repository name. But if it doesn't, that would've been the likely cause of the mismatch.

cc @woodruffw WDYT?

lwasser commented 8 months ago

hey @webknjaz πŸ‘‹ i can absolutely implement the security fix but first i just want to get the workflow - working. when you have a moment can you please have a look at this:

https://github.com/pyOpenSci/pyosMeta/actions/runs/8065805669

this wasn't working prior to adding the environment.

i have tried

  1. double and triple checking trusted publisher setup in pypi wondering if i spelled something wrong
  2. using an enviornment and not using an environment
  3. i did have to add that key for permissions - so i did that

I can play with the case sensitivity but on github's end lower case and caps both work resolving the url to the build so i'm not confident that is the issue but perhaps it is!

One other thought i had - if there aren't issues with my publish workflow, is the repository did used to have a different name. i can't remember when i changed it - perhaps a month or so ago?

i'm sorry i linked to the incorrect workflow above - i was typing too quickly. i've been fighting with this for a few days and can't understand how to get it to work. I'm looking for the simplest implementation to get started (within separating creating the build and publishing). but i'll absolutely separate for the final go once i understand what the issue is here. many thanks!

lwasser commented 8 months ago

btw i've easily gotten the token approach to work before but feel like trusted publisher might be a better / simpler implementation route.

woodruffw commented 8 months ago

I'm not sure if the PyPI normalizes the repository name. But if it doesn't, that would've been the likely cause of the mismatch.

Hmm, I think we do, but I'll check.

woodruffw commented 8 months ago

@webknjaz Good eye, I think you're right πŸ˜… -- we seem to take the user's repository without normalizing the case at all. I think we originally did that because GitHub doesn't document whether repository names are intended to be case insensitive or not (in practice they're insensitive e.g. in URLs, but this seemingly isn't guaranteed).

@lwasser Sorry for the mess here! You've hit a bug in PyPI πŸ™‚ -- I believe @webknjaz is right that changing your Trusted Publisher configuration to pyosMeta for the repository name will work around the bug here. Meanwhile, I'll look into a proper fix on PyPI's side.

woodruffw commented 8 months ago

Filed https://github.com/pypi/warehouse/issues/15498 for the upstream bug.

webknjaz commented 8 months ago

I can play with the case sensitivity but on github's end lower case and caps both work resolving the url to the build so i'm not confident that is the issue but perhaps it is!

Yeah, GitHub has HTTP redirects in place. However it still uses the repo slug with its canonical spelling in the bundle of data it signes on the OIDC level. So the PyPI gets proof that it's coming from pyOpenSci/pyosMeta specifically (this is printed out in your error/debug output). And that's what's signed and trusted. My concern was that the PyPI might not normalize this data and what you entered there when setting up trust before comparison, which is why I asked William to double check. From the action perspective, this seems to be configured and working as expected. But the mismatch needs to be tracked down somehow. Perhaps, a case could be made for adding additional hints to the error output and to the guide. Beyond that, we don't have visibility into why matching fails β€” only the PyPI does (to the extent of what's logged).

@woodruffw it occurred to me the maybe the PyPI could print out the expected data it's going to match, in the same key-value format the action prints it out now. That would be useful, right?

P.S. Pro tip: @lwasser while testing the integration, I recommend using PEP 440 pre-releases for versions, like dev/alpha/beta etc. This would allow you to avoid burning through the stable version bumps. And once it works, you'd cut the stable release.

lwasser commented 8 months ago

ok friends. that was indeed it. πŸš€ I missed it too - case sensitivity! many thanks! i think i just got tired of trying things last night but today with fresh eyes i just fixed the trusted publisher definition in PyPI to directly match the name - case sensitive pyosMeta. It just ran AND published to PyPI successfully. πŸŽ‰

@woodruffw it occurred to me the maybe the PyPI could print out the expected data it's going to match, in the same key-value format the action prints it out now. That would be useful, right?

my two cents: A note about case sensitivity in the output that the action provides would be very useful. i have noted it myself however. and the explanation makes a lot of sense.

thank you both and please don't apologize. i know how much work this is and also you're dealing with someone who is new to trusted publishing.

i will look into dev / pre releases as well to simplify and avoid that process i keep doing of making and deleting releases which is not good! many thanks. i am happy to close this now.

woodruffw commented 8 months ago

@woodruffw it occurred to me the maybe the PyPI could print out the expected data it's going to match, in the same key-value format the action prints it out now. That would be useful, right?

Just to make sure I understand: where are you thinking this print would happen? I suppose we could shoe-horn it into the response that twine renders, but that might be a bit of a hack πŸ™‚


Thank you for confirming the fix @lwasser! Glad to hear it's working.

I think the case sensitivity is strictly a bug on PyPI's side, so I'll try and have a fix merged today. That should replace the need for notes about sensitivity, since it'll no longer be sensitive πŸ™‚

lwasser commented 8 months ago

of course. please let me know if i can help in any way with testing things.

willingc commented 8 months ago

@webknjaz and @woodruffw Thank you so much for the thoughtful messages and for unblocking @lwasser. :heart:

webknjaz commented 8 months ago

@lwasser thanks for confirming, I'm glad you got it working! It is now also fixed in Warehouse (the PyPI engine) via https://github.com/pypi/warehouse/pull/15501.

Just to make sure I understand: where are you thinking this print would happen? I suppose we could shoe-horn it into the response that twine renders, but that might be a bit of a hack πŸ™‚

@woodruffw I was thinking about the UI in Warehouse specifically, not Twine β€” on the trusted publishing page. This is because the uploading client can't learn this information from the PyPI when the PyPI can't match it in the first place. But the end-user could use some help with things to check/compare that could be displayed where they configured trust.

The GHA workflow currently prints out the following on the console (and job summary):

sub: repo:pyOpenSci/pyosMeta:ref:refs/tags/v0.2.2
repository: pyOpenSci/pyosMeta
repository_owner: pyOpenSci
repository_owner_id: 28938222
job_workflow_ref: pyOpenSci/pyosMeta/.github/workflows/publish-pypi.yml@refs/tags/v0.2.2
ref: refs/tags/v0.2.2
But the PyPI shows the trust information as a table like this: Publisher Details
GitHub Repository: pyOpenSci/pyosMeta
Workflow: publish-pypi.yml
Environment name: (any)

Here's the idea: what if that https://pypi.org/manage/project/pyosmeta/settings/publishing/ page were to display the details closer to how the action present them? Perhaps, not in place of this table, but additionally. Maybe in some collapsed "debug" section.

It could then give out instructions to the end users on what to check for in the following format with placeholders for uncertain/variable data but exact values for things it knows it expects from a trusted connection:

sub: repo:pyOpenSci/pyosMeta:ref:refs/XXX
repository: pyOpenSci/pyosMeta
repository_owner: pyOpenSci
repository_owner_id: 28938222
job_workflow_ref: pyOpenSci/pyosMeta/.github/workflows/publish-pypi.yml@refs/XXX
ref: refs/XXX

It could explain that XXX are placeholders but the rest of the data must match exactly, for example. It could maybe spell out what it expects for the environment being set vs not.

I believe this could be a useful debugging tool / checklist for when people first try out setting us tokenless publishing. And with the data being displayed in the same format+order, it would hint the users to compare the bits separately.

As a more involved idea there could be a "debugging" interface in Warehouse, next to the trust configuration with an input where people could copy-and-paste what the action outputs in GHA and that thing could run diff with what it expects highlighting what doesn't match. I think this would be rather cool to have, but is probably not something the developers would be able to dedicate resources to implement. I still wanted to record these thoughts, though…

woodruffw commented 8 months ago

Sorry for the delayed response, lost this in my tabs πŸ˜…

@webknjaz Gotcha, that makes more sense! More debuggability would be great on the Warehouse side, but I think there are some risks to presenting the exact claims for users to fill in: technically the claims themselves are an implementation detail, so rendering them (with potential placeholders) potentially means surfacing OIDC changes to users whenever GitHub decides to update their claim set. In practice this should be pretty uncommon, but I think it pierces a layer of abstraction in a way that requires a bit of design thought.

In the mean time though, I think we can definitely improve the UX here: right now the GITHUB_STEP_SUMMARY includes the claims as a Markdown list, but we could make it into a table with the same/similar ordering as on PyPI to help with visual comparisons. If that sounds good as a starting point, I'm happy to send a PR with that change!