Skip to content

Add DELETE /api/v1/trusted_publishing/tokens API endpoint #11234

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

Draft
wants to merge 22 commits into
base: main
Choose a base branch
from

Conversation

Turbo87
Copy link
Member

@Turbo87 Turbo87 commented May 24, 2025

This PR adds an endpoint to revoke a temporary access token from the Trusted Publishing flow.

The DELETE /api/v1/trusted_publishing/tokens endpoint expects the token to be handed over in the Authorization header as a Bearer token, similar to how it will be used in the publish endpoint.

This PR is based upon (and currently includes the changes of) #11131, which implements the API endpoint to create a temporary access token (from a JWT).

Related:

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

Would you be willing to also add some regression tests? Specifically around cross-crate and user manipulation. This could probably apply to some of the other endpoints as well.

Id feel better just covering any future errors.

@Turbo87
Copy link
Member Author

Turbo87 commented May 27, 2025

absolutely, but could you be slightly more specific on the exact scenarios you would like to see tested? :)

@Turbo87 Turbo87 force-pushed the trustpub-revoke branch from 5699949 to 1141dd8 Compare May 28, 2025 07:39
Turbo87 added 21 commits May 28, 2025 11:04
This fn can be used to decode a JSON web token without verifying it's signature or claims. Only the `iss` claim will actually be decoded, since we use that to find the correct decoding key for the JWT issuer.
This defaults to the domain name (crates.io / staging.crates.io) and controls the expected `aud` claim of the OIDC JWT in the Trusted Publishing token exchange.
This makes it possible to construct `MockTokenUser` instances from an existing plaintext token or other random header value.
@Turbo87 Turbo87 force-pushed the trustpub-revoke branch from 1141dd8 to a9cde09 Compare May 28, 2025 09:06
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 incredibly minor notes, but no blocking concerns. Great work! 👍

Comment on lines +62 to +64
// If we're in a cooldown from a previous refresh, return
// whatever is in the cache.
return Ok(cache.keys.get(key_id).cloned());
Copy link
Contributor

Choose a reason for hiding this comment

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

Practically, it seems unlikely that the cache will now contain key_id, since we just did a check a moment ago under a read lock. (Obviously, in theory another task could acquire the write lock and refresh between releasing the read lock and acquiring the write lock, but that feels unlikely in practice.)

I'm not sure the behaviour should change, but I think the comment maybe should:

Suggested change
// If we're in a cooldown from a previous refresh, return
// whatever is in the cache.
return Ok(cache.keys.get(key_id).cloned());
// If we're in a cooldown from a previous refresh, return
// whatever is in the cache, which will probably be None
// given the previous check under the read lock.
return Ok(cache.keys.get(key_id).cloned());

Comment on lines +72 to +75
if let Some(key_id) = &key.common.key_id {
let decoding_key = DecodingKey::from_jwk(&key)?;
cache.keys.insert(key_id.clone(), decoding_key);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

kid is technically optional in JWKs. Given that we're requiring a key ID in any actual requests this is fine in practice, but I wonder if we should warn if we see a key that doesn't have an ID, since that may indicate future problems with that provider.

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