Skip to content
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

Check for existing Trusted Publishers before constraining existing one #17576

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

facutuesca
Copy link
Contributor

Overview

This PR makes the following changes:

  • Add a missing check when constraining a Trusted Publisher from "no environment" to a specific environment
  • Adds extra information to the OIDCPublisherAdded event, to make debugging easier

Missing check

When a user uses a Trusted Publisher with no environment configured, from a CI running in a specific environment, we send them an email suggesting they constrain the TP to only accept the specific environment they just used (see #17281).

The email contains a magic link to constrain the existing TP. However, if the user manually creates a constrained TP after the warning email was sent, but before the magic link is clicked, we get an uncaught exception due to not checking for existing TPs.

Concretely, here's the sequence of events that would trigger the issue:

  • User creates TP with (owner, repo, workflow), but no environment
  • User mints a token with that TP using environment my_env
  • User gets an email with a magic link to constrain TP to (owner, repo, workflow, my_env)
  • Before clicking the magic link, user manually creates another TP with (owner, repo, workflow, my_env)
  • Then user clicks the magic link to constrain the first TP, and gets an error because the constrained TP already exists (created manually in the previous step)

We add a check in constrain_environment() that checks if the constrained publisher already exists before trying to add it

Extra metadata in OIDCPublisherAdded

We add two new additional fields to the OIDCPublisherAdded event:

reified_from_pending_publisher: bool
constrained_from_existing_publisher: bool

These should be self-explanatory. Additionally, we now add an event when reifying a pending trusted publisher, since before there were no events for it.

cc @di @woodruffw

Signed-off-by: Facundo Tuesca <facundo.tuesca@trailofbits.com>
Signed-off-by: Facundo Tuesca <facundo.tuesca@trailofbits.com>
@facutuesca facutuesca requested a review from a team as a code owner February 7, 2025 17:42
Copy link
Member

@di di left a comment

Choose a reason for hiding this comment

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

LGTM, just one minor comment.

else:
self.request.session.flash(
"Can only constrain the environment for GitHub and GitLab publishers",
queue="error",
)
return self.default_response

# The user might have already manually created the new constrained publisher
# before clicking the magic link to constrain the existing publisher.
if self._check_publisher_exists_in_db(constrained_publisher):
Copy link
Member

Choose a reason for hiding this comment

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

Feels like this can be constrained_publisher.exists() instead or something similar, so a) the logic is per-publisher and b) this is easy to re-use elsewhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done! See the last commit

Signed-off-by: Facundo Tuesca <facundo.tuesca@trailofbits.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants