-
-
Notifications
You must be signed in to change notification settings - Fork 936
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
WIP: Add Buildkite as a trusted publisher #5437
base: master
Are you sure you want to change the base?
Conversation
…IDC token Until recently, Buildkite OIDC tokens did not contain a `jti` claim. At some point in early 2024 it was possible to assume an API Key Role using Buildkite OIDC tokens, but when testing in January 2025 we found the assume role request was failing with an error: > Missing/invalid jti Buildkite has addressed that by adding a `jti` claim to tokens - it's a good claim to include. However, to reduce the risk of regressions in the future, this proposes adding an integration test with a Buildkite-shaped OIDC token. The trait added to the OIDC::Provider factory is based on a real token that I generated then anonymized. I only test the happy path with this token - there's a buncha existing tests for various unhappy paths (expired token, etc) using the Github Actions shaped OIDC token and there's little value in replicating them. Most of the added test is copy-pasted from the happy-path Github Actions test further up the file. Fixes rubygems#5412
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yob you're a hero for getting this started!
This is generally what I'd expect.
I assume it'd be OK to ship Buildkite Trusted Publisher support without Sigstore and attestation support, and that we could follow up with that in a later PR?
Yes, that is OK, so long as it fails with a 400 with a clear message (and that code path is tested)
Thanks to the polymorphic association (and the different shape of the OIDC tokens), the forms for creating a new trusted publisher for GitHub Actions and Buildkite are quite different. The current "new trusted publisher" form has a provider select with a single item, but unless we want to do javascript things it might be easier to ask the user to pick the provider prior to loading the new form. What do you think?
If it's not too hard with a stimulus controller, I imagined using the selector to change -- it could even re-navigate the page, setting a query param. That way, it remains possible to link directly to a "create a trusted publisher" form
|
||
def change | ||
create_table :oidc_trusted_publisher_buildkites do |t| | ||
t.string :organization_slug, null: false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are these two slugs both immutable, or is it possible for someone to take over an organization name that was previously owned by someone else?
@@ -14,22 +14,31 @@ def view_template | |||
title_content | |||
|
|||
div(class: "t-body") do | |||
p do | |||
"New Trusted Publisher: #{rubygem_trusted_publisher.trusted_publisher.class.publisher_name}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i18n ?
@@ -18,7 +18,8 @@ def view_template | |||
end | |||
|
|||
p do | |||
button_to t(".create"), new_rubygem_trusted_publisher_path(rubygem.slug), class: "form__submit", method: :get | |||
button_to t(".create") + " Buildkite", new_rubygem_trusted_publisher_path(rubygem.slug), params: {trusted_publisher: "buildkite"}, class: "form__submit", method: :get |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
instead of multiple buttons here, what if the trusted publisher new view reloaded the form when the selector for trusted publisher type was changed?
|
||
def to_access_policy(jwt) | ||
# TODO what to do with jwt here? | ||
# TODO should we be checking the audience claim? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, audience claim should be checked, same as is done for the GHA publisher
end | ||
|
||
def to_access_policy(jwt) | ||
# TODO what to do with jwt here? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we're OK to ignore it, since there are no cross-claim consistency checks we need to enforce, unlike with GitHub (where we make sure the workflow source matches up with the commit being built)
) | ||
end | ||
|
||
#class SigstorePolicy |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we add a push test that uses a build kite trusted publisher and includes an attestation?
I'm still learning my way around this codebase, so my approach for exploring trusted publishers and adding Buildkite has been to hack-and-slash my way through to a point where in my local environment I can exchange a Buildkite OIDC token for a trusted publisher API token via
/api/v1/oidc/trusted_publisher/exchange_token
💪Now that I'm here, I thought I'd come up for air and check in with y'all before I start to polish it up. Here's a few thoughts and questions I have: