Skip to content

controllers/krate/publish: Add support for Trusted Publishing access tokens #11294

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

Merged
merged 2 commits into from
Jun 7, 2025

Conversation

Turbo87
Copy link
Member

@Turbo87 Turbo87 commented Jun 4, 2025

@Turbo87 Turbo87 added C-enhancement ✨ Category: Adding new behavior or a change to the way an existing feature works A-backend ⚙️ labels Jun 4, 2025
@Turbo87

This comment was marked as outdated.

Copy link
Contributor

@LawnGnome LawnGnome left a comment

Choose a reason for hiding this comment

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

A couple of non-binding suggestions, but LGTM in general. I like the new trustpub test, in particular! :+1

@Turbo87 Turbo87 force-pushed the trustpub-publish branch from b6bb52b to ded9ab7 Compare June 6, 2025 08:47
@Turbo87 Turbo87 requested a review from LawnGnome June 6, 2025 08:49
@eth3lbert
Copy link
Contributor

🙋 I was wondering if we could use the creator of the trustpub config as published_by but then I realized we don't record that information 😅. Is this a bad idea, or are we not supposed to do it this way?

@Turbo87
Copy link
Member Author

Turbo87 commented Jun 6, 2025

I think that could be misleading, since it is not the person that triggered the release itself. It might be possible to figure out the releaser from the actor or actor_id fields of the JWT, but that seems like a future enhancement to me and it would also only work for GitHub Actions, but not for GitLab CI or other Trusted Publishers.

@eth3lbert
Copy link
Contributor

🙋 Another question came up. I noticed that only the regular (non-TrustPub) AuthType requires a verified email for publishing, while the TrustPub AuthType does not seem to require this. Would it be bad to ensure email is verified before creating a trustpub config?

@Turbo87
Copy link
Member Author

Turbo87 commented Jun 6, 2025

Would it be bad to ensure email is verified before creating a trustpub config?

yeah, we should probably implement that 👍

Turbo87 added 2 commits June 7, 2025 09:46
When "Trusted Publishing" is used we can no longer associate the release with a specific crates.io user account. The column is already nullable since old releases did not track the information, so there are no changes needed on the database side.
@Turbo87 Turbo87 force-pushed the trustpub-publish branch from ded9ab7 to a0348f9 Compare June 7, 2025 07:46
@Turbo87 Turbo87 enabled auto-merge June 7, 2025 07:51
@Turbo87 Turbo87 merged commit 8d89c7e into rust-lang:main Jun 7, 2025
10 checks passed
@Turbo87 Turbo87 deleted the trustpub-publish branch June 7, 2025 07:56
@eth3lbert
Copy link
Contributor

🙋 A random question just came to mind. IIUC, the supported issuer is checked during the key exchange stage. This means that once they've exchanged the token, they can then ensure to publish a new version with it this time, even if we were to roll back support for this issuer right after the exchange, right?

@Turbo87
Copy link
Member Author

Turbo87 commented Jun 7, 2025

they can then ensure to publish a new version with it this time, even if we were to roll back support for this issuer right after the exchange, right?

yes, that's how it's currently implemented. I guess if we wanted to change that we would need back-references from the temporary tokens to their configurations (i.e. one nullable column per provider?) and set them to "on delete cascade".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-backend ⚙️ C-enhancement ✨ Category: Adding new behavior or a change to the way an existing feature works
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants