-
Notifications
You must be signed in to change notification settings - Fork 968
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
Trusted publishing: Support for GitHub reusable workflows #11096
Comments
Just as a note: this will have to interoperate with #11263. |
Any chance to expedite this issue? It seems there's many who would be eager to move over to trusted publishing now that it is the recommended approach, but not being able to do the flow from a reusable workflow is a pretty major blocker. This issue also does not seem to be documented anywhere so we only find out about it once we changed everything to use the new approach. |
I can't personally promise a timeline here, but it is on my backlog of things to do.
Thank you for pointing this out; I'll make some changes to the docs to emphasize that reusable workflows are not currently supported. |
I've opened #13592 for this. Thanks again for bringing it to our attention! |
This reverts commit d4e6137. Blocked by pypi/warehouse#11096.
This reverts commit f23a477. Blocked by pypi/warehouse#11096.
Switch to using API Token pending pypi/warehouse#11096
@woodruffw I just stumbled upon an example of a reusable workflow actually working with OIDC here jorisroovers/gitlint#486 — could you confirm this is because of |
That workflow is in the same repository though, I think the issue is with reusable workflows that are in a different repository (e.g. in a separate public repo). |
Yeah, I think that's because it's in the same repo, not because of In other words: reusable workflows do work with the current implementation, just not reusable workflows that are external to the repository that the trusted publisher was configured with. |
Yes, you can't use |
You can get around the |
The current version of the PyPI GitHub Action deploy workflow assumes the calling workflow is in the repo to be deployed when using Trusted Publishing: pypi/warehouse#11096 As such, putting the deploy workflow in a shared location is not supported, and leads to errors.
The current version of the PyPI GitHub Action deploy workflow assumes the calling workflow is in the repo to be deployed when using Trusted Publishing: pypi/warehouse#11096 As such, putting the deploy workflow in a shared location is not supported, and leads to errors.
The current version of the PyPI GitHub Action deploy workflow assumes the calling workflow is in the repo to be deployed when using Trusted Publishing: pypi/warehouse#11096 As such, putting the deploy workflow in a shared location is not supported, and leads to errors.
The deploy workflow cannot be shared because the PyPI GitHub Action deploy workflow cannot be shared when using Trusted Publishers. See pypi/warehouse#11096
I've been looking into how we can implement this. I've written up the relevant facts about how GitHub and PyPI currently handle OIDC claims, and at the end I specify what adding support for reusable workflows would look like in light of those facts. GitHub OIDC claims with (non-)reusable workflows
PyPI verification of OIDC claims for GitHub workflows
Note how our checks in (11) and (12) mix claims about the bottom-level workflow ( Note also that we accidentally support a subset of reusable workflows: when the top-level and bottom-level workflow are located in the same repo, and the bottom-level workflow is called in the same branch/tag/sha as the top-level one, then the OIDC claims check will pass. Concretely, if the claims look like this: "workflow_ref": "org/repo/.github/workflows/top-level-workflow.yml@refs/heads/main",
"job_workflow_ref": "org/repo/.github/workflows/reusable-workflow-3.yml@refs/heads/main",
"ref": "refs/heads/main", and the user configured the workflow filename as Adding support for reusable workflowsSo, what does all of this mean? First, we want to change the existing OIDC claims verification so that it checks against the top-level workflow instead of the bottom-level one (from Second, in order to fully support reusable workflows we need the user to input 3 new pieces of information: the owner and name of the repository where the reusable workflow lives, and the filename of said workflow. Once we have that, we can compare incoming OIDC tokens against the old information (owner, repo and filename of top-level workflow) and against the new information (owner, repo and filename of the bottom-level workflow). Third, note how we can trivially migrate existing Trusted Publishers without user intervention, by just copying the existing 3 fields into the new ones, since for non-reusable workflows they should be equal. And fourth, from the web UI perspective, the user should not need to fill the 3 new fields when creating a Trusted Publisher for a non-reusable workflow. Rather, those new fields can be hidden behind a "Reusable workflow" checkbox, and when left empty they will default to the same values as the old 3 fields (for the same reason as the point above). cc @woodruffw |
Thank you for the detailed write-up, @facutuesca! 😃 🍰 Is there a reason why we cannot match on |
The scenario we're trying to avoid is having overscoped Trusted Publishers (which is why we suggest having a dedicated, publish-only workflow). In the case of only checking for top-level workflows, imagine you have a project with a top-level
If we only match on |
@facutuesca and the environment name matching remains the same, right? |
For my use-case, I've been holding off using trusted publishing in environments that have reusable workflows until this issue is solved, so I won't have any opinion on the migration path. If you think it would be valuable to employ trusted publishing on such a repo, I'd be open to exploring that and providing feedback on the migration path, but my preference would be to await a durable solution. |
@webknjaz Yes, the environment check will still be against the
Ah I think I wasn't super clear there. I was referring to the migration from the POV of PyPI: when adding the 3 new columns to the database, we can fill the values with a copy of the existing information (owner, repo, filename). This will be completely transparent to the users, they won't need to migrate or change anything. |
Thanks, that makes sense! I'm not fully convinced though:
Is there anything terribly wrong with changing the check PyPI is currently doing from Footnotes
|
True, but I would argue that a parent-level workflow will be overscoped almost by definition, whereas a non-reusable workflow can be made minimally scoped. The argument is: if you only check the top-level I think it makes sense there to give the user the ability to narrow it down to a single reusable workflow.
True, but only for those who want to set up reusable workflows (the three new fields would be hidden until the user checks the checkbox). So the user experience for the currently supported use case (non-reusable workflows) should be the same, and the added complexity is for the users who want to use the new feature (reusable workflows)
This might be an option. IME though, most projects don't use environments, so making it required would (arguably) be harder for users since they would have to set it up. I'm open to it though, since I might be wrong here.
As you mentioned, one issue is breaking currently working Trusted Publishers, which we'll see if it's actually a problem once we have those metrics. The other issue is the discussion we've been having above: if we only check for |
Reusable workflows can be made minimally scoped just as well. You may be right that folks are more likely to overscope here, but I don't have any data on this. 🙂
FWIW I think security-wise environments would be the superior solution here. For example, we have (relatively speaking) lots of folks with write access to the main branch, but only a few selected individuals can approve deployments (and thus push to PyPI). The one major downside I can think of is that environments are not available for free accounts in private repositories. So that makes it hard to enforce as a strict requirement. If that's a major concern, Thank you for pushing this entire thing forward! 🎉 |
I'd definitely prefer the implementation where we can scope on the reusable workflow level, I think that the few optional fields in the setup UI would be totally fine and it's unlikely to confuse anyone. Environments are great, but there are lots of organizations that already have an established CI/CD setup that does not use environments and have no need for it otherwise, so requiring it just to use this feature would be probably suboptimal. |
With attestation becoming a more and more important part of the publication process to PyPI I wanted to ask if it would be possible to expedite this work together with #13911. This issue makes it less convenient to use trusted publishing compared to the token-based approach (which works fine with reusable workflows already), while the lack of tag pattern matching described in #13911 makes it difficult to achieve the same level of control and security over publishing as what the token-based approach provides. |
We're currently working on reusable workflow support. There are no short-term plans (on my part, at least) for #13911, however. That's going to require a larger re-thinking of the data model, since every additional optional field geometrically increases the number of states PyPI needs to check. |
Our MVP implementation (#10753) assumes that the workflow is in the same repository, which is not necessarily true.
We should support reusable workflows, specifically via the
job_workflow_ref
claim.The text was updated successfully, but these errors were encountered: